Display relevant Security Advisories data for Drupal

My understanding is that https://www.drupal.org/u/beautifulmind is working on this patch. I cannot assign because they are not a maintainer.

Changes have been committed and here is the patch. Please review and confirm.

Updating status based on the patch being uploaded. Will review when I can.

Thanks!
-Derek

+++ b/core/modules/update/src/Services/Notify.php
@@ -0,0 +1,167 @@
+  protected function doSend($to, array $params) {
+    $users = $this->entityTypeManager->getStorage('user')
+      ->loadByProperties(['mail' => $to]);
+    foreach ($users as $user) {
+      $to_user = reset($users);
+      $params['langcode'] = $to_user->getPreferredLangcode();
+      $this->mailManager->mail('update_psa', 'notify', $to, $params['langcode'], $params);
+    }
+  }

$to comes from config, but this won't send a mail if the email address doesn't have an account.

Also, the loop over $users isn't right, $user isn't even used inside the loop.

Finally, the first parameter to $this->mailManager->mail() should be the module name, "update_psa" isn't a module.

Thank you for the review. I will do another iteration by fixing all the issues and will submit for the community review.

Regards.

@beautifulmind thanks for getting this started!

Not a full review but a few things

1. --- /dev/null
   +++ b/core/modules/update/config/install/update_psa.settings.yml
   
   +++ b/core/modules/update/config/install/update_psa.settings.yml
   @@ -0,0 +1,14 @@
   
   @@ -0,0 +1,14 @@
   +psa_endpoint: 'https://updates.drupal.org/psa.json'
   +enable_psa: true
   +notify: true
   +check_frequency: 43200
   +enable_readiness_checks: true
   +hashes_uri: 'https://updates.drupal.org/release-hashes'
   +ignored_paths: "modules/*\nthemes/*\nprofiles/*"
   +download_uri: 'https://www.drupal.org/in-place-updates'
   +enable_cron_updates: false
   +enable_cron_security_updates: false
   +database_update_handling:
   +  - maintenance_mode_activate
   +  - execute_updates
   +  - maintenance_mode_disactivate
   

   I think these settings can just live in core/modules/update/config/install/update.settings.yml

   But we actually don't need most of these. We only need the settings that relate to the actual PSA functionality. These are all the settings for the contrib module but we are not adding all of the functionality here.

2. +++ b/core/modules/update/config/schema/update.schema.yml
   @@ -40,3 +40,43 @@ update.settings:
   +update_psa.settings:
   +  type: config_object
   +  label: 'Automatic updates settings'
   +  mapping:
   +    psa_endpoint:
   +      type: string
   +      label: 'Endpoint URI for PSAs'
   +    enable_psa:
   +      type: boolean
   +      label: 'Enable PSA notices'
   +    notify:
   +      type: boolean
   +      label: 'Notify when PSAs are available'
   +    check_frequency:
   +      type: integer
   +      label: 'Frequency to check for PSAs, defaults to 12 hours'
   +    enable_readiness_checks:
   +      type: boolean
   +      label: 'Enable readiness checks'
   +    hashes_uri:
   +      type: string
   +      label: 'Endpoint URI for file hashes'
   +    ignored_paths:
   +      type: string
   +      label: 'List of files paths to ignore when running readiness checks'
   +    download_uri:
   +      type: string
   +      label: 'URI for downloading in-place update assets'
   +    enable_cron_updates:
   +      type: boolean
   +      label: 'Enable automatic updates via cron'
   +    enable_cron_security_updates:
   +      type: boolean
   +      label: 'Enable automatic updates for security releases via cron'
   +    database_update_handling:
   +      type: sequence
   +      label: 'Database update handling'
   +      sequence:
   +        type: string
   +        label: 'Tagged service to handle database updates'
   \ No newline at end of file
   

   Same here. We only need the schema for PSA functionality. probably just the first 4 items. I think we can nest these for under the existing schema in the file under their own collectoin "psa"

3. +++ b/core/modules/update/src/Services/AutomaticUpdatesPsa.php
   @@ -0,0 +1,252 @@
   +use Psr\Log\LoggerInterface;
   

   Not used. Don't need a use statement if only referenced in a comment.

4. +++ b/core/modules/update/src/Services/AutomaticUpdatesPsa.php
   @@ -0,0 +1,252 @@
   +/**
   + * Class AutomaticUpdatesPsa.
   + */
   +class AutomaticUpdatesPsa implements AutomaticUpdatesPsaInterface {
   

   The class name should just be UpdatesPsa because we are adding this to the Update module directly.

   Also the class needs description of what it does instead of the current comment

5. +++ b/core/modules/update/src/Services/AutomaticUpdatesPsa.php
   @@ -0,0 +1,252 @@
   +    $this->profile = $profile;
   +    $this->theme = $theme;
   ...
   +    return $this->{$extension_type}->exists($project_name) && !empty($this->{$extension_type}->getAllAvailableInfo()[$project_name]['version']);
   

   This dynamic accessing of the properties was fixed in #3157857: Do not accessed Extensionlist class variables in AutomaticUpdatesPsa dynamically

   @beautifulmind were working from the 8.x-1.x branch of the contrib module or 8.x-2.x just before this was commited? either is fine because I don't think there has been that many changes to this functionality.

   but I think we should bring in the changes from that issue.

6. +++ b/core/modules/update/src/Services/AutomaticUpdatesPsaInterface.php
   @@ -0,0 +1,18 @@
   diff --git a/core/modules/update/src/Services/Notify.php b/core/modules/update/src/Services/Notify.php
   
   diff --git a/core/modules/update/src/Services/Notify.php b/core/modules/update/src/Services/Notify.php
   new file mode 100644
   
   new file mode 100644
   index 0000000000..66b9355f1c
   
   index 0000000000..66b9355f1c
   --- /dev/null
   
   --- /dev/null
   +++ b/core/modules/update/src/Services/Notify.php
   

   I think this patch would be easier to review if we did the mailing in a follow up.

   but it is probably a product/release management question as to whether we can have the notification only displayed on the screen as first step.

7. +++ b/core/modules/update/src/Services/Notify.php
   @@ -0,0 +1,167 @@
   + * Class EmailNotify.
   

   Need a class description

8. +++ b/core/modules/update/src/Services/NotifyInterface.php
   @@ -0,0 +1,15 @@
   + * Interface NotifyInterface.
   

   Need an actual description

9. +++ b/core/modules/update/update.install
   @@ -177,3 +178,31 @@ function _update_requirement_check($project, $type) {
   +  $requirements['update_psa'] = [
   +    'title' => t('<a href="@link">Public service announcements</a>', ['@link' => 'https://www.drupal.org/docs/8/update/automatic-updates#psas']),
   

   Should change to :link instead of @link because it is in html.

   But actually this shouldn't be link because it links the contrib module docs.

10. +++ b/core/modules/update/update.module
    @@ -19,6 +19,8 @@
    +use Symfony\Component\Process\PhpExecutableFinder;
    +use Symfony\Comopenent\Process\Process;
    

    Not used

11. +++ b/core/modules/update/update.module
    @@ -173,6 +185,11 @@ function update_cron() {
    +  ¶
    

    trailing space

12. +++ b/core/modules/update/update.services.yml
    @@ -22,3 +22,24 @@ services:
    +      - '@string_translation'  ¶
    

    trailing space

Would someone who is "in the know here" be able to provide a concise and clear summary of what's trying to be accomplished in this issue? I'm struggling to understand how exactly this will differ from what's already in Core (as in, Core already has update status screens + update status emails).

Thanks,
-Aaron

Hello @tedbow
Thank you for the detailed review. I noticed those points and will make enhancements as well.

Initially, I wanted to combine those settings but for the sack of clarity, I created a separate file under /install!

In the next iteration, most probably by Monday evening or Tuesday morning, I will enhance the code further and submit a patch for community review.

Once again, I really appreicate your detailed review & feedback. Thank you. :)

Regards.

All the issues have been fixed, logger added, PSA Notification message template added. Fixed all the Coding standards issues. Please review the patch.

Regards.

Marking NR for #14.

I just cleaned the files for white spaces. Should I upload the cleaned patch with same file name? Or should revise the file name with target comment number? Please advise.

Regards.

Not a core developer, but new comment number is always better (you can track just the whitestpace changes in an interdiff.txt or interdiff-oldno-newno.txt).

Here is the cleaned patch.

Regards.

@beautifulmind Thanks for uploading that. Would you be willing to also post interdiffs with your patches, so folks can see what's changed between each version?

Thanks!
-Derek

Certainly! I will post the interdiffs as well.

Didn't mean to change the status.

Here is the interdiff.

On a side note, when I tried executing a test from CLI on my local environment, I accidentally deleted everything under /core, losing my work! But then I had the patch created just before that. I ran composer update, and then applied the patch! I have everything back!

But here, it keeps failing, throwing different kinds of errors. Any thoughts?

I figured it out!

1. +++ b/core/modules/update/src/ProjectInfoTrait.php
   @@ -0,0 +1,197 @@
   +      if (!in_array($extension_type, $this->getExtensionsTypes())) {
   +        throw new \UnexpectedValueException("Invalid extension type: $extension_type");
   +      }
   +      $list = \Drupal::service("extension.list.$extension_type");
   

   We no longer need this logic because the only class that uses this trait is UpdatesPsa and it should always have $this->extensionLists[$extension_type] set. If not should throw the exception

2. +++ b/core/modules/update/src/ProjectInfoTrait.php
   @@ -0,0 +1,197 @@
   +  protected function getExtensionsTypes() {
   +    return ['module', 'profile', 'theme'];
   +  }
   

   this can be remove too

3. +++ b/core/modules/update/update.module
   @@ -173,6 +183,10 @@ function update_cron() {
   +    // Checkers should run before updates because of class caching.
   

   This comment I think refers to the readiness checkers. This is copied directly from the contrib module but it looks to me like the contrib module had this comment on the wrong line.

4. +++ b/core/modules/update/update.routing.yml
   @@ -84,4 +84,4 @@ update.confirmation_page:
   -    _access_update_manager: 'TRUE'
   +    _access_update_manager: 'TRUE'
   \ No newline at end of file
   

   Unrelated change

5. +++ b/core/modules/update/src/Services/Notify.php
   @@ -0,0 +1,169 @@
   +  /**
   +   * Event dispatcher.
   +   *
   +   * @var \Symfony\Component\EventDispatcher\EventDispatcherInterface
   +   */
   +  protected $eventDispatcher;
   

   Unused

6. +++ b/core/modules/update/src/Services/UpdatesPsa.php
   @@ -0,0 +1,238 @@
   +namespace Drupal\update\Services;
   

   It doesn't make sense to just add all the new services to a "Services" namespace. The update module already has a punch of services. I think we should create a new "Psa" namespace

7. I noticed the functional tests from the contrib module were not move over. I copied them over pretty much as is except for moving the PSA related test methods into 1 test class.

   I didn't review them yet but I wanted to bring them to make sure this is working. I had to make some more changes to make the tests pass.

8. A bunch of tests were failing because the update settings and schema files didn't pass

+++ b/core/modules/update/src/ProjectInfoTrait.php
@@ -0,0 +1,181 @@
+      // From 8.8.0 onward, always use packaged for core because non-packaged
+      // will no longer make any sense.
+      if (version_compare(\Drupal::VERSION, '8.8.0', '>=')) {
+        $infos['drupal']['packaged'] = TRUE;
+      }

We can refactor away this check, because this will always be true.

@tedbow Thank you for the suggestions. I fixed all the issues and will remove unused code and comments.

About the readiness check comment, I kept it thinking it will help implementing that feature when we arrive there. But for now, I will remove it.

@longwave Thank you for the suggestion. I will accomodate this in the patch.

@beautifulmind sorry I was unclear I fixed the issues I commented on. See the interdiff file that I included with #26

About the readiness check comment, I kept it thinking it will help implementing that feature when we arrive there. But for now, I will remove it.

I removed that but also the readiness checks are being worked on here #3162655: Create Automatic Updates Readiness Checks in new experimental module. They are being implemented in a new experimental module.

UPDATE: sorry I guess I didn't actually remove this comment

This patch adds 2 things

1. Cleans up the test class added in #26. The class was mostly just copied from the contrib module in #26
2. Fixed the 2 test failures in #26 by copying the new template added to both the stable and stable9 themes

@tedbow Thank you for the update and clarification.

Regards.

1. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
   @@ -0,0 +1,239 @@
   +      $version_array = explode('-', $version, 2);
   +      if ($version_array && $version_array[0] === \Drupal::CORE_COMPATIBILITY) {
   +        return isset($version_array[1]) ? $version_array[1] : NULL;
   +      }
   +      if (count($version_array) === 1) {
   +        return $version_array[0];
   +      }
   +      if (count($version_array) === 2 && $version_array[1] === 'dev') {
   +        return $version;
   +      }
   +    }, $json->insecure));
   +    $version_array = explode('-', $extension_version, 2);
   

   We should consider expanding \Drupal\update\ModuleVersion to handle this parsing.

   We added this class we intentionally made cover only the parsing we needed at the time and made @internal

   But since this is still in the Update module it would not affect the @internal status.

   UPDATE: actually this is dealing with versions in $json->insecure which we can't rely on.

   So probably don't need this whole method.

2. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
   @@ -0,0 +1,239 @@
   +    $contrib_constraint = $parser->parseConstraints($current_version);
   +    if ($psa_constraint->matches($contrib_constraint)) {
   +      $messages[] = $this->message($json->title, $json->link);
   +    }
   

   I think the logic here is wrong.

   looking at the docs for the the PSA feed and the insecure property it says

   List of versions of the affected project that are currently insecure. This does not indicate which versions will be marked as insecure. This list will be updated after the security release is published, to also include newly-insecure versions.

   So in the code we are only adding messages if current version is within insecure. We should not be filtering by this.

   By policy for security releases the versions that will be affecting cannot be disclosed before hand.

   So I don't think we actually need this at all.

   UPDATE: so reading this code again I see it only matches the version if it isn't a PSA. I am not sure what other types of message are going to be in the JSON fee but currently the docs don't say the insecure changes for non-PSA's. I have pinged @drumm and @heddn about this.

3. Until I get clarification on insecure I am going to assume the current functionality is correct.

   +++ b/core/modules/update/src/Psa/UpdatesPsa.php
   @@ -0,0 +1,239 @@
   +          if ($json->is_psa && ($json->type === 'core' || $this->isValidExtension($json->type, $json->project))) {
   +            $messages[] = $this->message($json->title, $json->link);
   +          }
   +          elseif ($json->type === 'core') {
   +            $this->parseConstraints($messages, $json, \Drupal::VERSION);
   +          }
   +          elseif ($this->isValidExtension($json->type, $json->project)) {
   +            $this->contribParser($messages, $json);
   +          }
   

   I still think the logic here is confusing.

   First we call $this->isValidExtension($json->type, $json->project) 2x. I think we should just call it once an at the top of this block and just continue if it is FALSE.

   2nd, parseConstraints() doesn't really "parse constraints". $json->insecure just as an array of versions only internally in parseConstraints() does it turn them into constraints, so from the caller perspective the name doesn't make sense. Also it calls \Composer\Semver\VersionParser::parseConstraints() which uses the same method name but does actually just parse constraints.

   3rd in addition parseConstraints() also adds to the $messages array which is not clear at all reading this code block.

   4th, contribParser() has the same problem as as above but also determines the installed version of the project. So it does 3 things.

   I have refactored this to have 3 methods, matchesInstalledVersion() which will call getContribVersions() and getInstalledVersion()

   I think this will be clearer because the 3 methods can be clearer in what the do.

   Then this code block can just be

   if ($json->type !== 'core' && !$this->isValidExtension($json->type, $json->project)) {
     continue;
   }
   if ($json->is_psa || $this->matchesInstalledVersion($json)) {
     $messages[] = $this->message($json->title, $json->link);
   }
   

   To me this makes it more clear that we add messages for all PSA's and for other types we only add the message if it matches the installed version.

   I wasn't clear on this before.

1. Updated remaining tasks with an item to specify if we want to follow the same logic regarding items where is_psa !== true

2. +++ b/core/modules/update/tests/modules/psa_test/src/Controller/JsonTestController.php
   @@ -72,7 +72,7 @@ public function json() {
   -      'project' => 'token',
   +      'project' => 'layout_builder_st',
   

   Left this is because I was testing something. Fixed

More review

1. +++ b/core/modules/update/src/Psa/EmailNotify.php
   @@ -0,0 +1,161 @@
   +   * @param string $to
   +   *   The email address where the message will be sent.
   

   Since the the method is toSend I don't think we need to call this $to. $email seems better.

2. +++ b/core/modules/update/src/Psa/EmailNotify.php
   @@ -0,0 +1,161 @@
   +   * @throws \Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException
   +   * @throws \Drupal\Component\Plugin\Exception\PluginNotFoundException
   

   We don't need the throws if the method itself don't throw these. At least that seems to be the pattern in core.

3. +++ b/core/modules/update/src/Psa/EmailNotify.php
   @@ -0,0 +1,161 @@
   +    foreach ($users as $user) {
   +      $to_user = reset($users);
   

   The $user var is never used. Also there will always at most be 1 user matching an email. So we don't need the foreach loop.

4. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
   @@ -0,0 +1,248 @@
   +        foreach ($json_payload as $json) {
   +          if ($json->type !== 'core' && !$this->isValidExtension($json->type, $json->project)) {
   

   This class just accesses the properties $json directly with no documentation of what will be in the object.

   I have made a SecurityAnnouncement class that checks for the keys in its factory method throws an exception if the expected keys are not found.

5. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
   @@ -0,0 +1,248 @@
   +      $this->logger->error($exception->getMessage());
   +      $messages[] = $this->t('Drupal PSA endpoint service is malformed.');
   ...
   +    $psa_constraint = $parser->parseConstraints($version_string);
   

   Before the changes in this comment the only place this could be thrown is in calls to \Composer\Semver\VersionParser::parseConstraints(). i have checked the contrib and this is the same.

   This means the exception could be thrown if an item in the JSON has a bad value for insecure. If so then no items will be displayed. This seems very unlikely to happen because the PSA feed from drupal.org is unlikely to have this error.

   But in the unlikely even that this does happen where there is problem with 1 item do we want to not show any items in the feed?

   Since these at first will be used with highly critical updates we should probably error on the side of showing as much information as we can.

   There seem to be 2 options for handling items that will have this problem

   1) Just don't show the individual item that has the problem but show the other items.

   2) If the item's only problem is in the parsing of insecure assume when can't parse this that we should show the message. Because it would be better to show the user a highly PSA message that does not apply to them then to NOT show them one that doesn't

6. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
   @@ -0,0 +1,248 @@
   +    $versions = $json->type === 'core' ? $json->insecure : $this->getContribVersions($json->insecure);
   +    $version_string = implode('||', $versions);
   

   We concatenate the version into 1 constraint string here so that we can call \Composer\Semver\VersionParser::parseConstraints() one time.

   but this means that if any of the version strings are malformed an exception will be thrown.

   I also don't think think calling parseConstraints() 1 time vs once for every version will actually be much of a performance gain. Internally parseConstraints() will call itself recursively for each version anyways. It will probably actually be worse performing because it has to parse the string to get each version.

   Changing to call once for each version.

7. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
   @@ -0,0 +1,248 @@
   +    $installed_constraint = $parser->parseConstraints($this->getInstalledVersion($json));
   

   This would throw the exception if the installed version has an invalid constraint. Not sure how this could happen but maybe if the extension was patched or the extension was installed from a git clone.

   This could result in not showing a PSA because the installed version number has a problem.

   Again I think here we should error on the side of showing the PSA when we can't be sure.

8. +++ b/core/modules/update/src/UpdateSettingsForm.php
   @@ -88,6 +88,27 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    $form['psa']['psa.enable'] = [
   +      '#type' => 'checkbox',
   +      '#title' => $this->t('Show Public service announcements on administrative pages.'),
   +      '#default_value' => $config->get('psa.enable'),
   +    ];
   +    $form['psa']['notify.psa'] = [
   +      '#type' => 'checkbox',
   +      '#title' => $this->t('Send email notifications for Public service announcements.'),
   +      '#default_value' => $config->get('notify.psa'),
   +      '#description' => $this->t('The email addresses listed above will be notified.'),
   +    ];
   +
   

   These 2 settings don't work they way they are described currently.

   Because psa.enable is checked in \Drupal\update\Psa\UpdatesPsa::getPublicServiceMessages() and an empty array is returned if it is empty then this settings actually everyone where the messages might be shown, 1) on admin pages, 2) on the status report and 3) via email.

   I am changing psa.enable to only be checked in update_page_top() so that it only affects showing on admin pages like it is described here.

   You should be able to not show the messages on admin pages and still send emails, which the current functionality does not allow you to do.

   I think showing on the status report should not be able to be disabled. I don't think we allow that for available updates except by disabling the Update module.

9. +++ b/core/modules/update/src/UpdateSettingsForm.php
   @@ -140,6 +161,8 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +      ->set('psa.enable', $form_state->getValue('psa.enable'))
   +      ->set('psa.notify', $form_state->getValue('psa.notify'))
   
   +++ b/core/modules/update/tests/src/Functional/PsaTest.php
   @@ -0,0 +1,147 @@
   +    $this->config('update.settings')
   +      ->set('psa.endpoint', $this->workingEndpoint)
   +      ->set('psa.enable', FALSE)
   +      ->save();
   

   The form setting didn't actually work here I think because of the period. The tests still passed because we were setting directly instead of using the form. I have fixed the form and changed the test to use the form.

10. We need tests for malformed Json

1. Adding test coverage for invalid JSON. I don't think the way patch currently handles this is good.
2. I removed the psa_test module and instead added actual json test fixture files.
3. Currently if there is invalid json:

   On the status report page: It will display "1 urgent announcement requires your attention" and then "Drupal PSA JSON is malformed." in the list

   In the notify email:: The same behavior happens.

   This is because in \Drupal\update\Psa\UpdatesPsa::getPublicServiceMessages() the exceptions are caught and "Drupal PSA JSON is malformed." is returned as the only message. So no caller has a good way to tell that an error occurred.

   I think a better solution would be for getPublicServiceMessages() to throw the exception and the callers handle it.

   I will fix this in the next patch.

1. This changes so that \Drupal\update\Psa\UpdatesPsa::getPublicServiceMessages() throws exception rather than returns the error message in the announcement messages array. see #38.3

   This means the callers can respond to the exception. I updated the tests but \Drupal\Tests\update\Functional\PsaTest::testInvalidJsonEmail() will fail. I am not sure what do in this case. Should we send an email saying we couldn't fetch PSA information and the admin should check drupal.org? or should we not send an email and just wait until it can get valid PSA's

2. +++ b/core/modules/update/src/Psa/EmailNotify.php
   @@ -0,0 +1,157 @@
   +      $frequency = $this->configFactory->get('update.settings')->get('psa.check_frequency');
   ...
   +      if (($this->time->getRequestTime() - $last_check) > $frequency) {
   

   Here we check if should actually send the messages via email based on update_psa.notify_last_check but we have already called getPublicServiceMessages(). If we aren't going to send the emails there is no reason to call this.

   Moving this to the beginning of the function.

1. +++ b/core/modules/update/src/Psa/EmailNotify.php
   @@ -0,0 +1,159 @@
   +    $messages = $this->updatesPsa->getPublicServiceMessages();
   

   When calling here to send an email I decided to just log an error if the message could not be retrieved.

2. +++ b/core/modules/update/update.install
   @@ -177,3 +181,46 @@ function _update_requirement_check($project, $type) {
   +  catch (TransferException $transfer_exception) {
   

   This was happening in hook_requirements update phase so it was causing test to file. This only should happen in the runtime phase.

3. +++ b/core/modules/update/update.install
   @@ -177,3 +181,46 @@ function _update_requirement_check($project, $type) {
   +    $requirements['update_psa']['value'] = t('Unable to retrieve PSA information from ' . \Drupal::config('update.settings')->get('psa.endpoint'));
   

   This same message was being constructed in 2 places and now a third that am handling errors when trying to send an email. I create a helper static function \Drupal\update\Psa\UpdatesPsa::getErrorMessageFromException(). This can be used in callers in get the message to display.

4. Also fixed the standards issues in the last patch

1. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
   @@ -0,0 +1,292 @@
   +  protected function isValidExtension(string $extension_type, string $project_name) {
   +    try {
   +      $extension_list = $this->getExtensionList($extension_type);
   +      return $extension_list->exists($project_name) && !empty($extension_list->getAllAvailableInfo()[$project_name]['version']);
   +    }
   

   🙀I just realized that this logic is assuming the project value that comes back in the PSA feed will be the same as the extension(module,theme,etc) name. This is usually true but not always. The project name is short machine name that is on Drupal.org. It does not have to be used for the extension name.

   changing to use the UpdateManager instead since it returns an array keyed by project name. This is more expensive but since this is not called often it should be ok.

2. Add a test case where the project does not match the extension name.

3. +++ b/core/modules/update/tests/fixtures/psa_feed/valid.json
   +++ b/core/modules/update/tests/fixtures/psa_feed/valid.json
   @@ -0,0 +1 @@
   

   I realize now the reason we can't use the json is because we can't hardcode for the core matching version. As it is now this would fail in 9.2.x. Changing back to use the controller.

   We could use the JSON if we dynamically set the core version which is what we use in other update tests. But changing to use the controller for now.

1. +++ b/core/modules/update/src/Psa/EmailNotify.php
   @@ -0,0 +1,178 @@
   +    $notify_emails = $this->configFactory->get('update.settings')->get('notification.emails');
   +    if (!empty($notify_emails)) {
   

   This should be checked at the beginning of the method instead because we can't do anything if we don't have any emails. Fixed

2. +++ b/core/modules/update/src/UpdateSettingsForm.php
   @@ -88,6 +88,27 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    $form['psa']['psa_notify'] = [
   +      '#type' => 'checkbox',
   +      '#title' => $this->t('Send email notifications for Public service announcements.'),
   +      '#default_value' => $config->get('psa.notify'),
   +      '#description' => $this->t('The email addresses listed above will be notified.'),
   +    ];
   

   This checkbox can be checked even if there are no emails provide. Adding validation to prevent this.

3. +++ b/core/modules/update/update.module
   @@ -173,6 +197,10 @@ function update_cron() {
      $interval = 60 * 60 * 24 * $frequency;
      $last_check = \Drupal::state()->get('update.last_check', 0);
      if ((REQUEST_TIME - $last_check) > $interval) {
   +    // Checkers should run before updates because of class caching.
   +    /** @var \Drupal\update\Psa\NotifyInterface $notify */
   +    $notify = \Drupal::service('update.psa_notify');
   +    $notify->send();
        // If the configured update interval has elapsed, we want to invalidate
   

   $notify->send() should not be within this if statement. Here $frequency is from check.interval_days setting which is how ofter to check for updates not PSA's.

   We could take it out of the loop and just call it every time cron runs because \Drupal\update\Psa\EmailNotify::send() already checks psa.check_frequency.

   but this still is not great because that means that if items stay in the PSA feed for more than 12 hours the admin would be sent the same message every 12 hours.

   So it think EmailNotify should not check how long it has been but checks if the last messages it sent are different than the messages it got back from \Drupal\update\Psa\UpdatesPsa::getPublicServiceMessages()

   getPublicServiceMessages() is already using a cache so it won't check that often.

   Changing testPsaEmail() to test that a email doesn't be sent if the messages have not changed and that the email is sent when the message does change.

   Using \Drupal\update_test\Datetime\TestTime to test the cache expires and the feed is checked again. But until #3113971: Replace REQUEST_TIME in services fixed this will not work. So copying just the changes to \Drupal\Core\Cache\DatabaseBackend that are needed for now. Hopefully we can get that issue committed soon.

4. Added remaining tasks item to make a list of bugs fixed and other improvements to backport to contrib module

1. coder fixes
2. Added 2 items in "Remaining tasks"
3. Not sure why the test failed in #43. I confirm it passed locally and fails on drupalci. Probably has something to do with faking the time. This patch will fail the same way.

Updated summary with a list of improvements made here from the contrib module that should be considered for the contrib module.
The project name !== extension name is probably the most critical

1. I could not figure why the tests in #43 and #46 fail on drupalci but not locally. I did a lot testing but I could not figure it out.
2. I am changing \Drupal\update\Psa\UpdatesPsa::getInstalledVersion() get the core version that way it does for other projects from info returned by \Drupal\update\UpdateManagerInterface::getProjects(). This how the Update module currently determines the core version to determine if there available updates.

   This allows using actually json files instead of the controller class to return the json feed because we can alter the version of core like the patch does with other modules. This is also more like using the XML test fixtures that the Update already use.

   This should make the tests pass.

3. More testing 😱
   For some reason the tests don't pass on drupalci because the mocking of the date to trigger the json PSA feed to be fetched again when the cache expires. This works locally but not on drupalci. If anybody could see #43 or #46 pass locally for them that would be be great.

   So I have changed the test to manually delete the updates_psa cache to trigger fetching of the PSA.

   Probably we don't need to do 1) to make the tests pass but I think using actual json files is better anyways. Also getting the core version the same way we do for other projects and the same way that other parts of the update module do is better.

1. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
   @@ -0,0 +1,309 @@
   +        $response = $this->httpClient->get($psa_endpoint)
   +          ->getBody()
   +          ->getContents();
   

   After a ton of more debugging it seems this line is the problem.

   I looked at \Drupal\update\UpdateFetcher::fetchProjectData() I noticed it did not call getContents() but instead casts the result of getBody() to a string.

   I am not sure why but I am guessing because \Psr\Http\Message\StreamInterface::getContents() says it

   Returns the remaining contents in a string

   So if it has been called before you have to call \Psr\Http\Message\StreamInterface::rewind() set it back to the beginning of the stream. Not sure why that would be different on drupalci.

   @longwave and @laurii also tested by debug patch and they saw the same fails.

2. +++ b/core/modules/update/tests/src/Functional/PsaTest.php
   @@ -0,0 +1,280 @@
   +    // Deleting the PSA cache will not result in another email if the messages
   +    // have not changed.
   +    $this->cache->delete('updates_psa');
   

   Added a @todo here for #3113971: Replace REQUEST_TIME in services so that we can update this test to just mock the date and test the cache gets cleared

updating the summary

Let's review this in the UX meeting next week.

Thoughts on the points in the IS:

1. I don't think we need this setting. The announcements are rare as it is and less config == better.
    

2. I agree using notification.emails would make sense (rather than a new setting) for the reasons mentioned.
    

3. Let's file an issue for this against d.o. Sometimes the PSA is about an upcoming release, but not always. And I agree for UX reasons we would ideally stop displaying the message once the user has updated to the version that addresses the issue. I think this could be a followup.
    

4. Per @drumm the only way these items get into the feed is via a Drush command that someone with prod access on d.o runs. I think that's extreme enough that we should display everything in the feed for a project in the site's codebase. I asked @drumm if it could be a UI config option on PSAs, but even then, we should trust the sec team's judgement that the item is worth displaying on the status report. PSAs are not typically assigned a severity so I don't think we need extra logic for that.
    

5. I agree that we should display the PSA for anything in the site codebase, installed or not. As discussed, even uninstalled modules can expose the site to security risks for certain vulnerabilities. I can think of several such vulnerabilities in the past 5 years.

I think NW to address points 1, 2, and 4. There are also a number of CS errors:

FILE: ...aintainer/core/modules/update/config/install/update.settings.yml
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 15 | ERROR | [x] Expected 1 newline at end of file; 2 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 56ms; Memory: 8MB

PHPCS: core/modules/update/config/schema/update.schema.yml passed
PHPCS: core/modules/update/src/Psa/EmailNotify.php passed
PHPCS: core/modules/update/src/Psa/NotifyInterface.php passed
PHPCS: core/modules/update/src/Psa/SecurityAnnouncement.php passed
PHPCS: core/modules/update/src/Psa/UpdatesPsa.php passed
PHPCS: core/modules/update/src/Psa/UpdatesPsaInterface.php passed

FILE: ...jm/git/maintainer/core/modules/update/src/UpdateSettingsForm.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 1 LINE
----------------------------------------------------------------------
 145 | ERROR | [x] No space found after comma in argument list
 145 | ERROR | [x] Expected one space after the comma, 0 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 62ms; Memory: 8MB

PHPCS: core/modules/update/tests/src/Functional/PsaTest.php passed

FILE: /Users/xjm/git/maintainer/core/modules/update/update.install
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 16 | WARNING | [x] Unused use statement
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 76ms; Memory: 8MB


FILE: /Users/xjm/git/maintainer/core/modules/update/update.module
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 23 | WARNING | [x] Unused use statement
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

1. +++ b/core/modules/update/src/Psa/EmailNotify.php
   @@ -0,0 +1,180 @@
   +class EmailNotify implements NotifyInterface {
   +  use StringTranslationTrait;
   

   I believe our coding standards specify a newline between the opening curly and the first line here (although it didn't fail phpcs).

2. +++ b/core/modules/update/src/Psa/EmailNotify.php
   @@ -0,0 +1,180 @@
   +   * @param \Drupal\Core\Mail\MailManagerInterface $mail_manager
   +   *   The mail manager.
   +   * @param \Drupal\update\Psa\UpdatesPsaInterface $updates_psa
   +   *   The automatic updates service.
   +   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
   +   *   The config factory.
   +   * @param \Drupal\Core\Language\LanguageManagerInterface $language_manager
   +   *   The language manager.
   +   * @param \Drupal\Core\State\StateInterface $state
   +   *   The state service.
   +   * @param \Drupal\Component\Datetime\TimeInterface $time
   +   *   The time service.
   +   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
   +   *   Entity type manager.
   +   * @param \Drupal\Core\StringTranslation\TranslationInterface $string_translation
   +   *   The string translation service.
   +   * @param \Psr\Log\LoggerInterface $logger
   +   *   The logger.
   

   Inane observation: That's a lot of services.

3. +++ b/core/modules/update/src/Psa/EmailNotify.php
   @@ -0,0 +1,180 @@
   +  public function send() {
   ...
   +  protected function doSend(string $email, array $params) {
   
   +++ b/core/modules/update/src/Psa/NotifyInterface.php
   @@ -0,0 +1,15 @@
   +  public function send();
   
   +++ b/core/modules/update/src/Psa/SecurityAnnouncement.php
   @@ -0,0 +1,206 @@
   +  public static function createFromArray(array $data) {
   ...
   +  protected static function validateAnnouncementData(array $data): void {
   
   +++ b/core/modules/update/src/Psa/UpdatesPsa.php
   @@ -0,0 +1,307 @@
   +  public static function getErrorMessageFromException(\Exception $exception, bool $throw_unexpected_exceptions = TRUE) {
   
   +++ b/core/modules/update/tests/src/Functional/PsaTest.php
   @@ -0,0 +1,283 @@
   +  public function testPsa() {
   ...
   +  public function testPsaMail() {
   ...
   +  public function testInvalidJsonEmail() {
   ...
   +  private function setSettingsViaForm(string $checkbox, bool $enable) {
   ...
   +  protected function getPsaEmails() {
   
   +++ b/core/modules/update/update.install
   @@ -177,3 +181,36 @@ function _update_requirement_check($project, $type) {
   +function _update_psa_requirements(array &$requirements) {
   

   Can we add return typehints where feasible?

4. +++ b/core/modules/update/src/Psa/EmailNotify.php
   @@ -0,0 +1,180 @@
   +    $messages_hash = hash('sha256', serialize($messages));
   +    // Return if the messages are the same as the last messages sent.
   +    if ($messages_hash === $this->state->get(static::LAST_MESSAGES_STATE_KEY)) {
   +      return;
   +    }
   

   Non-blocking: This seems like it is probably code repeated from elsewhere and probably should be API or internal functionality for the messaging service rather than hardcoded logic in the caller.

5. +++ b/core/modules/update/src/Psa/EmailNotify.php
   @@ -0,0 +1,180 @@
   +      '@count urgent Drupal announcement requires your attention for @site_name',
   +      '@count urgent Drupal announcements require your attention for @site_name',
   

   I think we have a practice of not putting the word "Drupal" in the UI anywhere since it can be branded as something else. Should we say "security announcements"?

6. +++ b/core/modules/update/src/Psa/EmailNotify.php
   @@ -0,0 +1,180 @@
   +   * @param array $params
   +   *   Parameters to build the email.
   
   +++ b/core/modules/update/src/Psa/SecurityAnnouncement.php
   @@ -0,0 +1,206 @@
   +  /**
   +   * The currently insecure versions of the project.
   +   *
   +   * @var array
   +   */
   +  protected $insecureVersions;
   ...
   +   * @param array $insecure_versions
   ...
   +   *   a list of versions that will be insecure when the security release is
   ...
   +   * @param array $data
   +   *   The security announcement data as returned from the JSON feed.
   ...
   +   * @param array $data
   +   *   The announcement data.
   
   +++ b/core/modules/update/update.install
   @@ -177,3 +181,36 @@ function _update_requirement_check($project, $type) {
   + * @param array $requirements
   + *   The requirements array.
   
   +++ b/core/themes/stable9/templates/admin/updates-psa-notify.html.twig
   @@ -0,0 +1,38 @@
   + * Available variables:
   + * - messages: The messages array
   

   Can you guess what I'm going to say? ;)

    
    
    
    
    
    

   As always, let's use a more specific data type for the array if possible, and describe and/or reference the canonical array structure for associative or mixed arrays.

   Also, no apostrophe in "PSAs".

7. +++ b/core/modules/update/src/Psa/NotifyInterface.php
   @@ -0,0 +1,15 @@
   + * Defines an interface for sending notification of update PSA's.
   
   +++ b/core/modules/update/tests/src/Functional/PsaTest.php
   @@ -0,0 +1,283 @@
   +    // No email should be sent if PSA's are disabled even the endpoint has
   

   Nit: there should not be an apostrophe in "PSAs". (Here and elsewhere in the patch where it's a plural rather than a possessive.)

8. +++ b/core/modules/update/src/Psa/SecurityAnnouncement.php
   @@ -0,0 +1,206 @@
   +        $volition_messages[] = (string) $violation;
   ...
   +      throw new \UnexpectedValueException(implode(",  \n", $volition_messages));
   

   I think they are violation messages, not volition messages? The words mean different things.

9. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
   @@ -0,0 +1,307 @@
   + * Service class to get Public Service Messages.
   

   Blah blah start with a verb etc.

10. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
    @@ -0,0 +1,307 @@
    +class UpdatesPsa implements UpdatesPsaInterface {
    +  use StringTranslationTrait;
    +  use DependencySerializationTrait;
    

    Leading space.

11. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
    @@ -0,0 +1,307 @@
    +   * This module's configuration.
    

    What module's configuration?

12. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
    @@ -0,0 +1,307 @@
    +   * The http client.
    

    Nit: "HTTP" should be uppercase in code comments.

13. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
    @@ -0,0 +1,307 @@
    +        $this->cache->set('updates_psa', $response, $this->time->getCurrentTime() + $this->config->get('psa.check_frequency'));
    

    Interesting. This seems like it should be stored in state rather than cache? Otherwise a cache clear blows it away and resets it.

    Also, we seem to be storing prognostication about when we should check next, rather than when we checked last. I'd expect the other way around, with a check on the other side to decide whether it's long enough ago relative to the config.

    However, we should primarily mimic whichever pattern the update module already does when checking for updates.

14. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
    @@ -0,0 +1,307 @@
    +          $this->logger->error('PSA malformed: ' . $unexpected_value_exception->getMessage());
    +          throw new \UnexpectedValueException($unexpected_value_exception->getMessage(), static::MALFORMED_JSON_EXCEPTION_CODE);
    ...
    +      $this->logger->error('Drupal PSA JSON is malformed: @response', ['@response' => $response]);
    +      throw new \UnexpectedValueException('Drupal PSA JSON is malformed.', static::MALFORMED_JSON_EXCEPTION_CODE);
    

    Logger and an exception? Do we really need to log messages about malformed PSAs? The site owner can't do anything about that.

    Come to think of it, should we even be throwing an exception? (For the same reason.)

15. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
    @@ -0,0 +1,307 @@
    +      $this->logger->error($exception->getMessage());
    

    Same question about the use of the logger.

16. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
    @@ -0,0 +1,307 @@
    +   * Determines if the Psa versions match for the installed version of project.
    

    Nit: "PSA" should be uppercase when used in a sentence.

17. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
    @@ -0,0 +1,307 @@
    +   * @param \Drupal\update\Psa\SecurityAnnouncement $sa
    +   *   The security announcement.
    ...
    +   * @param \Drupal\update\Psa\SecurityAnnouncement $sa
    +   *   The security announcement.
    ...
    +   * @param \Drupal\update\Psa\SecurityAnnouncement $sa
    +   *   The security announcement.
    ...
    +  private function getInstalledVersion(SecurityAnnouncement $sa) : string {
    

    An SA is not the same thing as a PSA. I think the parameter name should be $psa.

18. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
    @@ -0,0 +1,307 @@
    +   *   TRUE if security announcement matches the installed version of the
    +   *   project, otherwise FALSE.
    

    Nit: Comma splice. Either the word "or" or a semicolon would resolve this.

19. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
    @@ -0,0 +1,307 @@
    +      // If the installed version can not be parsed assume it matches to avoid
    

    Nit: "cannot" should be one word. (MURIKA!) Also, there should be a comma before "assume".

20. +++ b/core/modules/update/templates/updates-psa-notify.html.twig
    @@ -0,0 +1,38 @@
    +<p>
    +  {% trans %}
    +    A security update will be made available soon for your Drupal site. To ensure the security of the site, you should prepare the site to immediately apply the update once it is released!
    +  {% endtrans %}
    +</p>
    +<p>
    +  {% set status_report = path('system.status') %}
    +  {% trans %}
    +    See the <a href="{{ status_report }}">site status report page</a> for more information.
    +  {% endtrans %}
    +</p>
    +<p>{{ 'Public service announcements:'|t }}</p>
    +<ul>
    +  {% for message in messages %}
    +    <li>{{ message }}</li>
    +  {% endfor %}
    +</ul>
    +<p>
    +  {{ 'To see all PSAs, visit <a href="@uri">@uri</a>.'|t({'@uri': 'https://www.drupal.org/security/psa'}) }}
    +</p>
    +<p>
    +  {% set settings_link = path('update.settings') %}
    +  {% trans %}
    +    Your site is currently configured to send these emails when a security update will be made available soon. To change how you are notified, you may <a href="{{ settings_link }}">configure email notifications</a>.
    +  {% endtrans %}
    +</p>
    

    Let's get UX review for all this text.

    Also, I think we should not use the abbreviation "PSA" in the UI. We should always say "public service announcement".

21. +++ b/core/modules/update/tests/fixtures/psa_feed/invalid.json
    @@ -0,0 +1 @@
    +[{"title":"You can't parse this! Oh no! 🔥🙀🐶
    

    approve

22. +++ b/core/modules/update/tests/fixtures/psa_feed/valid.json
    @@ -0,0 +1,85 @@
    +      "8.5.14",
    +      "8.5.14",
    ...
    +      "8.6.15",
    +      "8.6.15",
    +      "8.5.15",
    +      "8.5.15",
    
    +++ b/core/modules/update/tests/fixtures/psa_feed/valid_plus1.json
    @@ -0,0 +1,96 @@
    +      "8.5.14",
    +      "8.5.14",
    ...
    +      "8.6.15",
    +      "8.6.15",
    +      "8.5.15",
    +      "8.5.15",
    

    Why are these versions in the list twice?

23. +++ b/core/modules/update/tests/fixtures/psa_feed/valid.json
    @@ -0,0 +1,85 @@
    +    "title":"AAA Update Project - Moderately critical - Access bypass - SA-CONTRIB-2019",
    ...
    +    "title":"AAA Update Test - Moderately critical - Access bypass - SA-CONTRIB-2019",
    ...
    +    "title":"BBB Update project - Moderately critical - Access bypass - SA-CONTRIB-2019",
    ...
    +    "title":"Missing Project - Moderately critical - Access bypass - SA-CONTRIB-2019",
    

    I know this is only test data, but the PSA title will always have a full date, not just a year. Otherwise we could only issue one per year.

24. +++ b/core/modules/update/tests/src/Functional/PsaTest.php
    @@ -0,0 +1,283 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  protected $defaultTheme = 'stark';
    +
    

    🤔 Do we need to override the default? (We already made Stark the default test theme in D9, right?) Does the theme even matter?

25. +++ b/core/modules/update/tests/src/Functional/PsaTest.php
    @@ -0,0 +1,283 @@
    +   * A test end PSA endpoint that returns invalid JSON.
    

    Nit: I think this is supposed to say "A test PSA endpoint" (not "A test end PSA endpoint").

26. +++ b/core/modules/update/tests/src/Functional/PsaTest.php
    @@ -0,0 +1,283 @@
    +  protected function setUp() :void {
    

    Missing space between colon and typehint.

27. +++ b/core/modules/update/tests/src/Functional/PsaTest.php
    @@ -0,0 +1,283 @@
    +        'version' => '9.11.0',
    

    Inane observation on test data: Hot damn, 9.11.0. This is a version that should never be released. 🤞

28. +++ b/core/modules/update/tests/src/Functional/PsaTest.php
    @@ -0,0 +1,283 @@
    +    // Test transmit errors with JSON endpoint.
    

    Not really a sentence.

29. +++ b/core/modules/update/tests/src/Functional/PsaTest.php
    @@ -0,0 +1,283 @@
    +    // Setup test PSA endpoint.
    

    Nit: "Set up" should be two words.

30. +++ b/core/modules/update/update.install
    @@ -177,3 +181,36 @@ function _update_requirement_check($project, $type) {
    +  $requirements['update_psa'] = [
    +    'title' => t('Public service announcements'),
    +    'severity' => REQUIREMENT_OK,
    +    'value' => t('No announcements requiring attention.'),
    

    I don't think we should display any message if there are no PSAs as it would be non-actionable clutter on the status report. We should validate this in the UX review, at least.

@xjm thanks for the review!

1. skipping #55 to fix clear code problems in this patch in next to comments. Will come back to this in next patch
2. Fixed phpcs problem in #56

re #57

1. fixed
2. I agree
3. Adding type hints. I couldn't type hint \Drupal\update\Psa\UpdatesPsa::getErrorMessageFromException() because it returns TranslatableMarkup|string
4. Not sure what you mean by "messaging service" here. Maybe \Drupal\update\Psa\UpdatesPsa? This is where $messages comes from.

   In this case I think it only makes sense for the email context not send the messages again. The other callers of \Drupal\update\Psa\UpdatesPsa::getPublicServiceMessages() probably still want to display the message even if they have been displayed before.

   Here the intention really is "don't email the same messages 2x". Only the caller here knows if they have been emailed.

5. fixed
6. Changed type array type hints to more specific such as string[]
7. fixed
8. fixed for this and the other classes
9. fixed
10. Changed 'update.settings' because there could be other configuration added for the update module.
11. fixed
12. The Update module currently uses \Drupal\Core\KeyValueStore\KeyValueStoreExpirableInterface::setWithExpire() to store available release on a per project basis in \Drupal\update\UpdateProcessor::processFetchTask()

    Changing to use that here but simpler because we are only dealing with 1 feed.

13. I have remove the logging for the exceptions.

    I still think we should throw the exceptions. Maybe in the email notification and the displaying on the admin we don't need to say anything if the feed is malformed but I think on the status report we should. Hopefully this feed will never be malformed but if it is it also could be the case that there is something wrong with the fetching of json file on the clients network and not a problem with drupal.org. If we don't display a message on the status report the site admin would never know this and if I PSA actually was in the feed they won't get it.

    Also I think any caller of \Drupal\update\Psa\UpdatesPsa::getPublicServiceMessages() should be able to tell the difference of between a feed with no items and problem with the feed. Without the exception this is not possible.

14. logger removed
15. fixed
16. This will only be called if is_psa is false because it is called for via matchesInstalledVersion() from here
    

    if ($sa->isPsa() || $this->matchesInstalledVersion($sa)) {
              $messages[] = $this->message($sa);
    }
    

    Because if it is a PSA we don't care what the installed version because as the feed currently works insecure will not tell use for a PSA whether it will affect the installed version when the fix is actually released.

    Leaving as $sa for now.

17. fixed
18. fixed
19. changed to "public service announcement"
20. 😂👍
21. Not intentional, fixed
22. fixed
23. Yep still needed. If it is not set the test has error that points to this https://www.drupal.org/node/3083055
24. fixed
25. fixed
26. Anything can happen in the alternate test universe 😉
27. fixed
28. fixed
29. agreed, fixed

assigning to myself to address #55

whoops here is the interdiff

back to #55

1. removed the setting
2. removed the setting
3. @todo Need to file the issue
4. Leaving as is
5. Add @todo in the code. Will need resolve before this patch is finished. Need a bigger change to not use project manager because of how it caches the results it won't work.

Taking care of #55.5 showing messages for extensions that are not installed.

This meant we can't use the UpdateManager to get projects because it caches the results and doesn't get uninstalled extensions unless the setting is enabled.

So had to change to not returning the test json files directly because we can't change the version of number of Drupal core. So added test controller to swap out the core version dynamically in the test json fixtures.

+++ b/core/modules/update/tests/fixtures/psa_feed/valid.json
@@ -0,0 +1,81 @@
+    "type":"theme",

This should have been 'module'. Didn't catch this before because \Drupal\update\UpdateManagerInterface::getProjects() returned all projects regardless of type and we weren't checking.

Some more self review

1. +++ b/core/modules/update/src/Psa/SecurityAnnouncement.php
   @@ -0,0 +1,206 @@
   +   * @param string $insecure_versions
   

   This should be string[]

2. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
   @@ -0,0 +1,308 @@
   +  protected function isValidProject(string $project_name, string $project_type) : bool {
   +    try {
   +      $project = $this->getProjectInfo($project_name, $project_type);
   +      return !empty($project['version']);
   +    }
   +    catch (\UnexpectedValueException $exception) {
   +      return FALSE;
   +    }
   +  }
   

   Here all we care about is the project version so we can remove this and just call the new getProjectVersion

3. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
   @@ -0,0 +1,308 @@
   +  private function getInstalledVersion(SecurityAnnouncement $sa) : string {
   

   We no longer are just worried about installed extension. Also here we actually are getting version constraint that we can compare. Changing the name to getExistingVersionConstraint()

4. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
   @@ -0,0 +1,308 @@
   +  protected function getProjectInfo(string $project_name, string $project_type): array {
   

   In the places this is called all we actually need is the project version. So changing this to getProjectVersion and returning an empty string if it is not available.

1. +++ b/core/modules/update/src/Psa/EmailNotify.php
   @@ -0,0 +1,183 @@
   +    if ($user = reset($users)) {
   +      $params['langcode'] = $user->getPreferredLangcode();
   +      $this->mailManager->mail('update', 'psa_notify', $email, $params['langcode'], $params);
   +    }
   

   This will only send an email if there is matching user. This is different than what _update_cron_notify() currently does. Changing to still send the email with the default language code if there is no user for the email.

2. +++ b/core/modules/update/src/UpdateSettingsForm.php
   @@ -88,6 +88,14 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    $form['psa'] = [
   +      '#type' => 'details',
   +      '#title' => $this->t('Public service announcements'),
   +      '#open' => TRUE,
   +    ];
   +    $form['psa']['description'] = [
   +      '#markup' => '<p>' . $this->t('Public service announcements are compared against the entire code for the site, not just installed extensions.') . '</p>',
   +    ];
   

   Removed the rest of the changes to the settings form since we have no new settings now

3. +++ b/core/modules/update/update.module
   @@ -19,6 +19,7 @@
   @@ -122,6 +123,24 @@ function update_page_top() {
   
   @@ -122,6 +123,24 @@ function update_page_top() {
   +
   +    /** @var \Drupal\update\Psa\UpdatesPsaInterface $psa */
   +    $psa = \Drupal::service('update.psa');
   +    try {
   +      $messages = $psa->getPublicServiceMessages();
   

   I just realized that we should be showing these messages on all admin page except the status report. We were excluding the same pages as available updates but that list for exclusion is based on where the user maybe seeing or uploading new updates.

4. Added screenshot for the UX review

Moving screenshots to the right section of the UI.

Wording changes from the UX meeting.

Fixing the sample email to not reference links that got removed.

Oops, I meant to decline credit, not assign to @tedbow.

Finally had a chance to closely review this. Mostly looking good. This is going to be a big improvement! Thanks to everyone who worked on this.

As is often the case when I review a huge patch, tons of pedantic nits, with a few items of substance...

1. +++ b/core/modules/update/config/schema/update.schema.yml
   --- /dev/null
   +++ b/core/modules/update/src/Psa/EmailNotify.php
   
   +++ b/core/modules/update/src/Psa/EmailNotify.php
   @@ -0,0 +1,165 @@
   +namespace Drupal\update\Psa;
   

   PSA is an acronym. What's the deal with namespaces and directories in this case? I guess core's "JsonApi" is an example of CamelCasing something that "should" be JSONAPI, so I guess this is fine. ;)

2. +++ b/core/modules/update/src/Psa/EmailNotify.php
   @@ -0,0 +1,165 @@
   +   * The automatic updates service.
   ...
   +   *   The automatic updates service.
   

   This doesn't (really) have anything to do with automatic updates, right?

3. +++ b/core/modules/update/src/Psa/EmailNotify.php
   @@ -0,0 +1,165 @@
   +   * @var \Drupal\update\Psa\UpdatesPsaInterface
   ...
   +  protected $updatesPsa;
   

   Curious why 'Updates' is plural here (and everywhere). We're adding this code to the 'Update Manager', not 'Updates Manager'...

4. +++ b/core/modules/update/src/Psa/EmailNotify.php
   @@ -0,0 +1,165 @@
   +   * Entity type manager.
   ...
   +   *   Entity type manager.
   

   Every other one of these var comments starts with 'The'.

5. +++ b/core/modules/update/src/Psa/EmailNotify.php
   @@ -0,0 +1,165 @@
   +      $this->logger->error($this->t(
   +        'Unable to send notification email because of error retrieving PSA feed: @error'),
   +        ['@error' => UpdatesPsa::getErrorMessageFromException($exception, FALSE)]
   +      );
   

   I'm probably tired and wrong, but it seems like the ['@error'] array is in the wrong place here. It's the 2nd arg to error(), not t()...

6. +++ b/core/modules/update/src/Psa/EmailNotify.php
   @@ -0,0 +1,165 @@
   +      $users = $this->entityTypeManager->getStorage('user')
   

   Do we want to call getStorage('user') once outside of this loop, instead?

7. +++ b/core/modules/update/src/Psa/NotifyInterface.php
   @@ -0,0 +1,15 @@
   +/**
   + * Defines an interface for sending notification of update PSAs.
   + */
   +interface NotifyInterface {
   

   Reading this patch from the top down, so maybe this is answered later, but I'm not clear why we need an interface for this. Are we expecting this to be a public API and contrib / custom are going to implement their own versions of this interface?

8. +++ b/core/modules/update/src/Psa/SecurityAnnouncement.php
   @@ -0,0 +1,206 @@
   + * These come form the PSA feed on drupal.org.
   

   s/form/from/

9. +++ b/core/modules/update/src/Psa/SecurityAnnouncement.php
   @@ -0,0 +1,206 @@
   +   * Whether this announce is PSA instead of another type of announcement.
   

   a) s/announce/announcement/

   b) s/is PSA/is a PSA/

   c) What other types of announcements are there? I'm not clear when this is ever FALSE.

10. +++ b/core/modules/update/src/Psa/SecurityAnnouncement.php
    @@ -0,0 +1,206 @@
    +  protected $isPsa;
    

    Pedantic aside: I still keep doing double takes on these. ;) Seems like isPSA would be more accurate...

11. +++ b/core/modules/update/src/Psa/SecurityAnnouncement.php
    @@ -0,0 +1,206 @@
    +   *   The version of the project that currently insecure. For PSAs this is not
    +   *   a list of versions that will be insecure when the security release is
    +   *   published.
    

    This comment doesn't parse. 1st sentence is missing 'is'. 2nd sentence 'not' throws me way off. What is this really trying to say?

12. +++ b/core/modules/update/src/Psa/SecurityAnnouncement.php
    @@ -0,0 +1,206 @@
    +   * @param mixed[] $data
    ...
    +   * @param mixed[] $data
    

    xjm will probably say "yes", but is 'mixed[]' any better or more specific than 'array' for these? ;)

13. +++ b/core/modules/update/src/Psa/SecurityAnnouncement.php
    @@ -0,0 +1,206 @@
    +    $new_blank_constraints = [
    ...
    +      new NotBlank(),
    

    Confused by an array called '$new_blank_constraints' with a NotBlank() constraint.

14. +++ b/core/modules/update/src/Psa/SecurityAnnouncement.php
    @@ -0,0 +1,206 @@
    +   * Whether the security announcement is PSA or not.
    

    s/is PSA/is a PSA/

15. +++ b/core/modules/update/src/Psa/SecurityAnnouncement.php
    @@ -0,0 +1,206 @@
    +   *   TRUE if the announcement is a PSA otherwise false.
    

    a) Needs a comma before 'otherwise'.
    b) s/false/FALSE/

16. +++ b/core/modules/update/src/Psa/SecurityAnnouncement.php
    @@ -0,0 +1,206 @@
    +   * Gets the currently insecure version of the project.
    

    s/version/versions/

17. +++ b/core/modules/update/src/Psa/SecurityAnnouncement.php
    @@ -0,0 +1,206 @@
    +   * Gets the links to the security announcement.
    

    s/links/link/

18. +++ b/core/modules/update/src/Psa/SecurityAnnouncement.php
    @@ -0,0 +1,206 @@
    +   *   The link.
    

    Maybe "The link to the PSA."?

19. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
    @@ -0,0 +1,285 @@
    + * Defines a service class to get Public Service Messages.
    

    s/Messages/Announcements/

20. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
    @@ -0,0 +1,285 @@
    +   * This 'update.settings' configuration.
    

    s/This/The/

21. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
    @@ -0,0 +1,285 @@
    +   * Update key/value store.
    

    Start with 'The'?

22. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
    @@ -0,0 +1,285 @@
    +   * The extension lists.
    ...
    +    $this->extensionLists['module'] = $module_list;
    +    $this->extensionLists['theme'] = $theme_list;
    +    $this->extensionLists['profile'] = $profile_list;
    

    Maybe worth documenting it's keyed by extension type?

23. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
    @@ -0,0 +1,285 @@
    +  public function getPublicServiceMessages() : array {
    ...
    +   * Gets a message from an exception thrown by ::getPublicServiceMessages().
    ...
    +   *   The exception throw by ::getPublicServiceMessages().
    

    s/Messages/Announcements/

24. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
    @@ -0,0 +1,285 @@
    +        if ($sa->getProjectType() !== 'core' && !$this->getProjectVersion($sa)) {
    +          continue;
    +        }
    

    I don't fully understand why we skip contrib SAs without a version. Maybe this needs a comment? Or maybe I'm misunderstanding...

25. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
    @@ -0,0 +1,285 @@
    +      throw new \UnexpectedValueException('Drupal PSA JSON is malformed.', static::MALFORMED_JSON_EXCEPTION_CODE);
    

    I wonder what a site admin is supposed to do when they see this exception message. ;)

26. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
    @@ -0,0 +1,285 @@
    +   *    Throw if the exception is not expected.
    

    s/Throw/Thrown/

27. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
    @@ -0,0 +1,285 @@
    +   * Determines if the PSA versions match for the installed version of project.
    

    wants 'a' before project. But then it'd be past 80 chars. Maybe:

    "Determines if the PSA versions match the installed version of a project." ?

28. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
    @@ -0,0 +1,285 @@
    +   *   TRUE if security announcement matches the installed version of the
    

    s/if security/if the security/

29. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
    @@ -0,0 +1,285 @@
    +   *   project; otherwise FALSE.
    

    s/;/,/

30. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
    @@ -0,0 +1,285 @@
    +   * @throws \UnexpectedValueException
    +   *   Thrown by \Composer\Semver\VersionParser::parseConstraints() if the
    +   *   constraint string is not valid.
    

    Do we document exceptions thrown by functions we call? That seems clumsy. I thought the whole point of exceptions is they can happen anywhere in the call stack...

31. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
    @@ -0,0 +1,285 @@
    +        // If an individual constraint is throws an exception continue to check
    +        // the other versions.
    

    This doesn't parse. Maybe:

    "If an individual constraint throws an exception, continue to check the other versions." ?

32. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
    @@ -0,0 +1,285 @@
    +   * Returns a message that links the security announcement.
    

    s/links the/links to the/

33. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
    @@ -0,0 +1,285 @@
    +   *   The PSA or SA message.
    

    Maybe: "A link to the upstream announcement using the message as the text." or something? It's not just returning the message, the interesting fact is that it's hard-coded to be a link tag.

34. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
    @@ -0,0 +1,285 @@
    +   * Gets the contrib version to use for comparisons.
    

    s/version/versions/

35. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
    @@ -0,0 +1,285 @@
    +   *   The versions that can be used for comparison.
    

    I'd love this comment to define what makes a version valid to use for comparison, so I wouldn't have to read the code to figure it out.

    Now that I've read the function, it seems more like a "Convert potentially BespokeVer Drupal versions into SemVer-esque versions for comparisons" function. Right? Maybe mention something about that?

    p.s. Don't we already have code like this somewhere else in Update Manager or in a core utility or component class? If not, shouldn't we?

36. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
    @@ -0,0 +1,285 @@
    +  private function getContribVersions(array $versions) : array {
    

    Why private instead of protected?

37. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
    @@ -0,0 +1,285 @@
    +      if ($version_array && $version_array[0] === \Drupal::CORE_COMPATIBILITY) {
    

    I thought we were trying to kill the CORE_COMPATIBILITY constant. ;)

38. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
    @@ -0,0 +1,285 @@
    +   * Gets the project version.
    

    Gets which project version? ;) How is this different from getExistingVersionConstraint()? This comment could use some clarification.

39. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
    @@ -0,0 +1,285 @@
    +   *   The project version or an empty string if the project is not available.
    

    What does "the project is not available" mean? Is this "the project is not installed"? "Not enabled"? Other?

40. +++ b/core/modules/update/src/Psa/UpdatesPsa.php
    @@ -0,0 +1,285 @@
    +    static $extensions = [];
    

    Static var since ExtensionList::getList() is too expensive? Seems like this will use a lot of RAM for something we're already storing in RAM. Perhaps worth a comment to justify this. Or maybe rip it out? Not sure...

41. +++ b/core/modules/update/src/Psa/UpdatesPsaInterface.php
    @@ -0,0 +1,18 @@
    + * Defines an interface to get Public Service Messages.
    ...
    +   * Gets public service messages.
    ...
    +  public function getPublicServiceMessages() : array;
    

    s/Messages/Announcements/

42. +++ b/core/modules/update/src/Psa/UpdatesPsaInterface.php
    @@ -0,0 +1,18 @@
    +   *   A array of translatable strings.
    

    A) s/A/An/
    B) Does this want to be more specific?

43. +++ b/core/modules/update/templates/updates-psa-notify.html.twig
    @@ -0,0 +1,35 @@
    + * Default theme implementation for the public service announcements email notification.
    

    Should we use 'PSA' here to avoid overflowing 80 chars?

44. +++ b/core/modules/update/templates/updates-psa-notify.html.twig
    @@ -0,0 +1,35 @@
    + * - messages: An array of message strings.
    

    Would maybe be better if this variable was 'announcements'...

45. +++ b/core/modules/update/templates/updates-psa-notify.html.twig
    @@ -0,0 +1,35 @@
    +    An important security announcement is available for your Drupal site. You should read the announcement immediately and follow its instructions.
    

    Should this care about plural?

46. +++ b/core/modules/update/templates/updates-psa-notify.html.twig
    @@ -0,0 +1,35 @@
    +<p>
    +  {% set settings_link = path('update.settings') %}
    +</p>
    

    Looks like this <p> is missing any visible payload.

47. +++ b/core/modules/update/tests/fixtures/psa_feed/valid.json
    @@ -0,0 +1,81 @@
    +    "insecure":[
    +
    +    ],
    

    Is this right?

48. +++ b/core/modules/update/tests/fixtures/psa_feed/valid.json
    @@ -0,0 +1,81 @@
    +      "8.x-8.7.0"
    ...
    +      "8.x-8.7.0"
    

    Is this here for things we can't parse?

49. +++ b/core/modules/update/tests/fixtures/psa_feed/valid_plus1.json
    @@ -0,0 +1,92 @@
    +    "title":"Critical Release - PSA because 2020",
    

    LMAO! ;) Love it!!

50. +++ b/core/modules/update/tests/modules/update_test/src/Controller/PsaTestController.php
    @@ -0,0 +1,34 @@
    +   * Gets the contents of JSON file.
    

    More like "Reads a JSON file and returns the contents as a Response." or something, right?

    Should it also mention the magic about replacing [CORE_VERSION] with the current version of core?

51. +++ b/core/modules/update/tests/modules/update_test/src/Controller/PsaTestController.php
    @@ -0,0 +1,34 @@
    +   *   The test response.
    

    Can this explain when you get a JsonResponse vs. a regular Response? Response is only for a 404 error condition for file-not-found, right?

52. +++ b/core/modules/update/tests/src/Functional/PsaTest.php
    @@ -0,0 +1,248 @@
    +    // Alter the 'aaa_update_test' to use the 'aaa_update_project' project name.
    +    // The PSA feed will match project name and not extension name.
    

    Not sure why we're doing this. Why not call the project name 'aaa_update_test' in the feed fixtures? Or this is here to test the case where the module name != project name? In that case, maybe spell that out more clearly in the comment?

53. +++ b/core/modules/update/tests/src/Functional/PsaTest.php
    @@ -0,0 +1,248 @@
    +   * Tests sending an email when the PSA JSON is invalid.
    

    Looks like it tests that we don't send email for malformed PSA JSON...

54. +++ b/core/modules/update/update.install
    @@ -177,3 +180,33 @@ function _update_requirement_check($project, $type) {
    + * Display requirements from Public service announcements.
    

    why capitalize Public?

55. +++ b/core/modules/update/update.services.yml
    --- /dev/null
    +++ b/core/themes/stable/templates/admin/updates-psa-notify.html.twig
    

    Same comments on this template above apply to these copies in stable and stable9...

@dww thanks for the review!

1. Yeah my understanding is acronyms still follow the same casing rule for class names and name spaces
2. fixed
3. Changing to Update singular

   Looking at the name of this service in particular I don't think we need "Update" in it at all. After talking with @xjm and other in security team, strictly speaking a PSA could be a notification about a security problem that can't be covered by an update.
   So changing this to UpdateFetcherInterface. I think the PSA feed simple enough that we don't need the "Processor" "Manager" and "Fetcher" that we do for the actual update XML logic.

   Other naming changes in this patch. Some related, some just giving harder look:

   • update-psa-notify.html.twig
   • changing from $this->tempStore->get('updates_psa'); to $this->tempStore->get('psa_response');. Here tempStore is already the update tempstore so we don't need to repeat it.
   • Service update.psa to update.psa_fetcher. We already have the update.psa_notify service so this makes it more clear what their responsibilities are.
   • logger.channel.update

Ok going to update with the above changes because the naming changes make a 13k interdiff and splitting it should make reviewing easier.

Assigning to myself because I am continue to address the rest of #71

Continuing with #71

4. Fixed and also a similar problem in the newly rename PsaFetcher
5. You are correct. But also I checked other calls to \Psr\Log\LoggerInterface::error() and they don't all a t() function. Removing
6. fixed
7. I also think it is better idea not to add a public API. see my previous comment under "API changes" in the summary.

   I am going to remove the 2 services that this patch adds.

   I think if there is case for custom/contrib code to need the interface the case can be made later. It seems like it would be such a rare case to want to want to change this functionality that it would be almost as simple for custom code to just fetch the json feed themselves.

   The problem in that case would be that our services would still be doing their thing so they might get duplicate messaging.

   I am going to add Drupal::hasService('update.psa_*')

checks so that custom code can still just unset our services if they want to write their own functionality to handle PSAs.

8. fixed
9. fixed a,b
   c, the current docs on https://www.drupal.org/docs/8/update/automatic-updates#s-public-service-... say

   The flag which indicates that the post is a PSA, and not another kind of Security Advisory.

   It could be just a "Security Advisory". I am not super clear on the distinction since by the very fact they are published make them all Public.
   I think the distinction is a PSA is something that is released before an upcoming release where as a SA if for a security vulnerability or fix that is already released.
   Maybe @xjm @drumm or somebody else from the security team could confirm this.

10. yep

    What I found https://www.drupal.org/docs/develop/standards/object-oriented-code#naming is

    If an acronym is used in a class or method name, make it CamelCase too (SampleXmlClass, not SampleXMLClass). [Note: this standard was adopted in March 2013, reversing the previous standard.]

    it doesn't explicitly say anything about properties. I did find \Drupal\jsonapi\Routing\Routes::$jsonApiBasePath where JSON and API are both acronyms.

    I couldn't think of other acronyms to grep for. Didn't find "xml" or "yml" used as properties.

11. changed to

       *   The versions of the project that are currently insecure. For PSAs this
       *   list does include versions that will be marked as insecure when the new
       *   security release is published.
    

    Basically PSA are for future releases, a security release will made available very soon.
    So if there is PSA for Drupal core, meaning a warning about an upcoming security release,
    $insecure_versions would contain all the versions from https://updates.drupal.org/release-history/drupal/current NOW that have the term, Release type == insecure.
    It does include versions will be insecure after the upcoming security release.

    We can't this in JSON feed because it is already used by the contrib module and also it could be used by other custom code.

12. i guess I would say "yes" also. I guess this is explicitly saying you know there will be elements of different types vs "array" might say that or it might say just forgot to update to something more specific
13. meant to be $not_blank_constraints

    fixed

14. fixed
15. fixed
16. fixed
17. fixed
18. fixed
19. fixed
20. fixed
21. fixed
22. fixed

Have to break. I will continue on with #71 after a couple hours

Continuing with #71 (thanks @dww a lot good points and questions!)

23. Skipping for now to avoid changing multiple times.

    Agreed it should be changed but I am trying to get clarification now as to what proper naming should be.
    on https://www.drupal.org/security/psa
    We have both "Security Advisory" and "Security Announcement". @drumm has said on drupal.org he is moving towards "Security Advisory" because it is more standard across projects.

    From the feed it seems that a PSA is special type of "Security Advisory". Right now the feed and the command on drupal.org to create the feed handle both.
    So we may want make more changes like change the newly renamed "PsaFetcher" to "SecurityAdvisoryFetcher" since it fetches both kinds.

24. This follows the logic of the contrib module where non-core item is shown if it doesn't pass isValidExtension() which requires a version number in the project.

    I think we should figure out what we want to do. for non-psa items the logic now is to only show items that have a match in insecure_versions. I think the reason here is because these aren't for future release so insecure_versions should reflect the versions that are affected and need to updated. So for non-psa items it may make sense to not show if the extension does not have version because we can't determine a match.

    On the other hand maybe we should default to showing non-psa items if we can't determine the existing version, just to be safe.

    Another problem here is that there maybe advisories that can't be solved with an update. Rather they may instruct the admin on some server config change or drupal config change that can't be automated(does someone know a previous example of this?). In that case insecure_versions wouldn't tell us anything. Maybe these will always be PSAs?

Because 24) raises some questions about how this should work and I have some other questions about the current version matching that was brought in from the contrib I am going to pause on address #71 and switch to writing a kernel test where we can clear test cases via a dataProvider. That way I think it will be easier for everyone looking at this patch to have the same understanding of when an item in the feed is displayed and when it is not displayed. We can determine if the current functionality is correct or if it should can change(I think it is probably not correct)

Couple of questions to address

1. if I extension does not have a version what should we do for non-psa and psa?
2. How do determine if an advisory is a current security vulnerability that can't be solved with an update? Will these happen?

   Will these always be is_psa === 1? If so it is easier because we don't need to worry about matching insecure_versions. If not I don't think we have enough information in the feed to tell use whether to check insecure_versions. We don't want to ignore insecure_versions because we don't want show message that we don't have to.

3. this relates to @dww's question in #71.35

   \Drupal\update\Psa\PsaFetcher::getContribVersions() has logic that converts Drupal module version numbers then in \Drupal\update\Psa\PsaFetcher::matchesInstalledVersion() we use \Composer\Semver\Constraint\ConstraintInterface::matches()

   This handles the case where the installed version is 8.x-1.0 but insecure_version contains 1.0 and the reverse case.
   Would this ever happen? I know why we do this conversion in other parts of the Update module but there we are dealing with greater than/less than versions to determine updates. But here we have exact versions that we are matching.

   We know the version number that should be in the project and we know how the insecure_version array is generated.
   In which cases would not be exact string matches of the versions?

   If we don't need this type of match we can really simplify the logic here.

   for know the test case I added will match 8.x-1.0 == 1.0 just to show the current functionality.

forgot @group on new test

+++ b/core/modules/update/tests/src/Kernel/PsaFetcherTest.php
@@ -0,0 +1,176 @@
+namespace Drupal\Tests\update\Unit;

I forgot to update the namespace after I changed this from a unit to a Kernel test.

We could make this a unit test but the mocking is pretty complicated. I think the kernel test is more readable and relies more on core real functionality.

1. Ok adding more kernel test cases. From talking to @drumm and @heddn I am not sure we want to match all the test cases we are currently matching. I am not changing any of the matching logic just adding test cases to make it clear how it currently works.

   Then I think we can simply the logic and remove the cases where it matches but should not.

2. The fail in #78 was a random test fail in an unrelated test. It passed when re-ran

1. +++ b/core/modules/update/src/Psa/PsaFetcher.php
   @@ -0,0 +1,288 @@
   +  protected function matchesInstalledVersion(SecurityAnnouncement $sa) : bool {
   

   Changing this to matchesExistingVersion as the extension might not be installed.

2. +++ b/core/modules/update/src/Psa/PsaFetcher.php
   @@ -0,0 +1,288 @@
   +    static $extensions = [];
   +    $project_type = $sa->getProjectType();
   +    if (!isset($extensions[$project_type])) {
   +      $extensions[$project_type] = $this->extensionLists[$project_type]->getList();
   +    }
   

   I don't think we need to worry about local caching for $extensions here. \Drupal\Core\Extension\ExtensionList::getList() already takes care of caching for us.

3. +++ b/core/modules/update/tests/src/Kernel/PsaFetcherTest.php
   @@ -0,0 +1,389 @@
   +      'contrib:not-exact:non-psa' => [
   +        'feed_item' => [
   +          'is_psa' => 0,
   +          'type' => 'module',
   +          'project' => 'the_project',
   +          'insecure' => ['1.0'],
   +        ],
   +        'existing_version' => '8.x-1.0',
   

   Ok I removing the logic from PsaFetcher that parses versions to match these non-exact versions.

   So it will only match if the existing version is in insecure.

   I chatted with @drumm and he said the version in insecure should be complete version strings.

4. I think the only reason we would want version parsing would be if a site is using a dev version of extension or core. I am not sure what we would want to show in this case. These of course will not have exact version matches in insecure. For items where is_psa == 1 this won't matter because we don't check insecure in that case.

   We could open a follow up to decide what we should do.

5. If we feel we can't release this feature without the ability to show items where is_psa === 0 and the site is using dev version of core or a contrib extension I think a solution would be to show all items where the major version of the local project(core or an extension) matches the major version of any version in insecure.

   I don't think we should try to match the minor version. Here is example situation why.

   • Say we find a security vulnerability now that affects both the latest release 9.0.7 and dev branch 9.1.x
   • We make SA-2020-123 when the security release 9.0.8 comes out. The problem is not critical so we don't release a PSA before.
   • We fix the problem in 9.1.x but since there haven't be any releases for that branch yet there is no release needed.
   • After 9.0.8 comes we realize the vulnerability could have been exploited in different way that actually is critical.. 9.0.8 is still not vulnerable so don't need another release
   • Because we realize the problem in SA-2020-123 was actually critical we update the text of that SA and add it to the JSON feed this patch fetches.
   • Now if you running 9.1.x-dev you may be vulnerable but no version in insecure would match your minor only your major.

   This example maybe seem far fetched but having any items in this feed seem like it will be very rare and we don't a chance to fix it if we mess up the logic before one gets added. I would rather error on this side of showing more messages to sties running dev version than having them miss critical SAs.

6. Now that I wrote that scenario out I realize the same situation could happen with the last minor branch of 9.x.x and the new dev branch 10.x.x before there are any release on 10.x.x. So in that case looking for major matches in insecure would not work.

   This could also be true contrib projects

7. It seems like the whole point of including insecure is that for cases where is_psa === 0 we know the versions that are affected and we only need to show the message to sites that have 1 of those versions. But sites running a dev version we really can't be sure about the relation between the code you are running and security vulnerability that is in one of the version in insecure

   I think for this reason probably the best solution is for projects where the site is running a dev version we show all items in the feed for that project regardless of the values of is_psa or insecure.

   I have not done this yet but will do it next.

I am leaving assigned to myself because I am still working on the patch but don't mean to stop anyone else from chiming in my last comments/patches.

+++ b/core/modules/update/config/schema/update.schema.yml
@@ -40,3 +40,13 @@ update.settings:
+      label: 'PSA settings'

+++ b/core/modules/update/src/Psa/EmailNotify.php
@@ -0,0 +1,165 @@
+  private const LAST_MESSAGES_STATE_KEY = 'update_psa.last_messages_hash';
...
+   * The PSA fetcher service.

+++ b/core/modules/update/src/Psa/PsaFetcher.php
@@ -0,0 +1,225 @@
+ * Defines a service class to get Public Service Announcements.
...
+class PsaFetcher {

+++ b/core/modules/update/update.install
@@ -177,3 +180,36 @@ function _update_requirement_check($project, $type) {
+    $requirements['update_psa']['title'] = t('Critical security announcements');

There tons of places in this patch that uses psa or "Public Service Announcement" where what is really being dealt with is a "Security Advisory" and some but not all of these "Public Service Announcement".

So removing "PSA" or "Public Service Announcement" except in the explicit places where we dealing with PSAs.

I know this causes a huge interdiff but it just doesn't make sense to a have PsaFetcher that is fetching and processing PSA and other types of Security Advisories.

Talking with @xjm, @mlhess and @drumm I confirmed that PSA are specific kind of Security Advisory. The feed https://updates.drupal.org/psa.json may actually contain both kinds.

I left "PSA feed" in because that is what is documented as.

Back to @dww review in 71

23. This is now \Drupal\update\SecurityAdvisories\SecurityAdvisoriesFetcher::getSecurityAdvisoriesMessages with the change in #81 to not use "Public Service Announcements" when their will be other kinds of advisories. I would like to keep "Messages" here because we are not returning all the data about the item just the messages.
24. This was following the contrib module logic where isValidExtension() would return FALSE if there was not "version" set in the info. If that makes sense we could leave the way it is. But I think for $sa->isPsa() === TRUE we should show the message regardless of whether we have a version or not since we don't check the version anyways. Adding back in getProjectInfo() we can use this to check if the project actually existing locally and skip if not. We can then call it from getProjectExistingVersion() which simplifies that method. Also checking !isset($this->extensionLists[$project_type]) directly here and taking it out later. If the project type is not valid we can't do anything.

    I have also added a new test case to \Drupal\Tests\update\Kernel\SecurityAdvisoriesFetcherTest::providerShowAdvisories() to confirm we show a PSA even if there is no local version set. There are already test cases to show we don't show a non-psa if there is no local version and we don't show either if the project does not exist.

25. Since my other changes I can't tell which of 2 times this appears in the class this is referring to now.

    If appears once here:

    try {
      $sa = SecurityAdvisory::createFromArray($json);
    }
    catch (\UnexpectedValueException $unexpected_value_exception) {
      throw new \UnexpectedValueException($unexpected_value_exception->getMessage(), static::MALFORMED_JSON_EXCEPTION_CODE);
    }
    

    At this we actually know it is valid JSON because json_decode() didn't return NULL. So the exception would only be thrown here if an individual item in the feed didn't pass the validation in \Drupal\update\SecurityAdvisories\SecurityAdvisory::validateAdvisoryData()

    I think in this case we should actually just skip the item and continue to the next item, if any. It seems very unlikely that the feed from drupal.org would have some valid and some not valid entries but if that were to happen for some reason we should process any items that we can because they may contain a critical advisory that should be displayed on their site. Removing the exception.

    The 2nd time this exception thrown in the class is if json_decode() returns a NULL meaning the json item is malformed. I added the code MALFORMED_JSON_EXCEPTION_CODE to try to distinguish this from a \UnexpectedValueException thrown when calling \Composer\Semver\VersionParser::parseConstraint() but since we no longer calling that because we are not doing version parsing then we don't need this code. Removing the code.

    I am going put off deciding what the admin should do with this error message until I relook at the part of the patch where the exceptions are handled. As far as the service is concerned I think it should throw an exception that has valid information. I think a caller of getSecurityAdvisoriesMessages() should be able to tell the difference between an empty array meaning there were no items that match conditions to show on the current site and the feed not being able to be processed.

26. fixed
27. Changed to "Determines if an advisory matches for the existing version of a project."
    Not using "PSA" anymore and using "existing" because the project may not be installed
28. fixed
29. fixed
30. You are correct but also no longer needed because we aren't calling parseConstraints()
31. No longer needed because we aren't calling parseConstraints()
32. fixed
33. fixed
34. No longer exists
35. Method has been removed see #80.3
36. Method has been removed see #80.3
37. Method has been removed see #80.3
38. I think this cleared up with new @return comment and the new name is getProjectExistingVersion(). getExistingVersionConstraint() has been removed
39. Changed to "The project version or an empty string if the project does not exist on the site."
    Trying not to use "installed" because now it just matters if the project exists in the code base not just if it is installed.
40. Removed because \Drupal\Core\Extension\ExtensionList::getList() handles caching. removed in #80.2
41. interface no longer exists
42. Fixed with more specific using your suggestions for the message() method above in 33). I am actually removing message() now because it is only called in 1 place and has no logic.
    I could be convinced no to remove it but it seems more readable.
43. changing to "security advisories" since it will not just be PSAs, fits in 80 now. Also changing the template name to update-advisory-notify.html.twig. Changed in other templates too
44. changed to "advisories".
45. add plural
46. removed. also removed See the <a href="{{ status_report }}">site status report page</a> for more information.. This was a recommendation from the UX that I forgot to implement. There is not actually more information on the status report page because it just has the same messages with links. Also if you happen to read the email after the items had been removed from the feed from drupal.org then the messages won't appear on the status report page at all which might make you think the problem doesn't exist which is probably not the case.
47. This test case shows that for is_psa === 1 the version in insecure are actually ignored. Because it a PSA is about future release. Hopefully the test cases in \Drupal\Tests\update\Kernel\SecurityAdvisoriesFetcherTest() make this more clear. We can't put test comment in a json file like we can XML ☹️
48. This is from the contrib project probably for parsing test. But we aren't parsing anymore, removing
49. 👍
50. fixed and added not about [CORE_VERSION]
51. yes. added a not explaining difference with a 404
52. Added more explanation
53. fixed
54. fixed , this is 'security advisories' now
55. updated twigs files with new changes.

A few other changes to replace psa with advisory where needed

1. changed the template names to update-advisory-notify.html.twig
2. changed the email key to advisory_notify
3. changed test controller to AdvisoryTestController

Unasigning myself as I am not working on the patch but I will now work on making sure the summary is update to date

Updating summary

1. cleaning up the summary more
2. Removing the follow up #3041885: Display relevant Security Advisories data for Drupal. chatted with @drumm and the PSA entries should be removed from the feed shortly after the security release is published. A regular security advisory will likely remain in the feed after the release which explain why the non-release version is critical. This way we don't have to worry about insecure in items with is_psa === TRUE

Found a few small problems

1. +++ b/core/modules/update/src/SecurityAdvisories/EmailNotify.php
   @@ -0,0 +1,165 @@
   +    $params['subject'] = new PluralTranslatableMarkup(
   +      count($messages),
   +      '@count urgent security announcement requires your attention for @site_name',
   +      '@count urgent security announcements require your attention for @site_name',
   +      ['@site_name' => $this->configFactory->get('system.site')->get('name')]
   +    );
   

   I think we should calling \Drupal\Core\StringTranslation\StringTranslationTrait::formatPlural() instead of using this class directly. I was trying to figure out why this class needed StringTranslationTrait. It does right now but I think using the trait is preferred. I see many more times in core.

2. +++ b/core/modules/update/src/SecurityAdvisories/SecurityAdvisoriesFetcher.php
   @@ -0,0 +1,219 @@
   +        catch (\UnexpectedValueException $unexpected_value_exception) {
   +          continue;
   +        }
   

   I added an explanation here as to why just skip items that are in the incorrect format.

   I don't think will ever actually but in the very slim chance it does we should display the items are in the correct format and error out if one is in an invalid format.

3. +++ b/core/modules/update/src/SecurityAdvisories/SecurityAdvisory.php
   @@ -0,0 +1,206 @@
   +        'is_psa' => new NotBlank(),
   

   This actually fails if the json has false. changing to handle values [1, '1', 0, '0', TRUE, FALSE].

   Right now in the test feed https://updates.drupal.org/psa-this-is-only-a-test.json
   it is using "1" but I think we should handle these other values in case for some reason, intentionally or not, the value type should changes.

   With this change I am creating a unit test for this class to fully test it.

4. +++ b/core/modules/update/tests/src/Functional/SecurityAdvisoryTest.php
   @@ -0,0 +1,251 @@
   +  use StringTranslationTrait;
   

   Trait not used.

5. +++ b/core/modules/update/update.install
   @@ -177,3 +180,36 @@ function _update_requirement_check($project, $type) {
   +function _update_psa_requirements(array &$requirements): void {
   

   changing function name to _update_advisories_requirements since these will not just be PSAs.

6. +++ b/core/modules/update/update.module
   @@ -70,6 +71,27 @@ function update_page_top() {
   +      catch (Exception $exception) {
   +        if (!($exception instanceof TransferException || $exception instanceof UnexpectedValueException)) {
   +          // If the exception is unexpected rethrow it.
   +          throw $exception;
   

   In this case the known exceptions that are instances of TransferException(could be subclasses) from \GuzzleHttp\Client::get() or UnexpectedValueException which is thrown if the json feed is malformed. We should match UnexpectedValueException exactly here and not its subclasses.

1. Addressing the last remaining question in the summary, how to handle a site using a dev release for is_psa === 0 item.

   Here is solution I propose

   If the site is using a dev release:

   If the is_psa === 1 then show the item. this is how the patch works currently

   If the is_psa === 0 then
   If the dev version only specifies a major version like 6.x-dev for branch using semantic versioning or 8.x-6.x for a branch using the legacy versions then
   Show the item if it matches the major version for any of the versions in insecure

   If the dev version specifies a major and minor version like 9.1.x-dev, for contrib or 9.1.0-dev, for core. then
   Show the item if it matches both the major and minor versions for any of the versions in insecure

2. I added getMinorVersion() to \Drupal\update\ModuleVersion so we don't have string parsing in the new class. getMinorVersion() was actually in \Drupal\update\ModuleVersion in a early version of #3074993: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates where we created the class but we removed it because it wasn't needed and the class is internal so weren't intending to make a class for use outside of core
3. See #80.4->7

for my previous concerns about dev version. I don't think there is perfect solution to this problem but I think matching as much as we can is good.

Update summary with dev version logic

@tedbow - Thanks for all your fantastic work in here to get this ready, and for so thoroughly and thoughtfully addressing all my review points!

I've carefully reviewed all your comments since #71, but not every single interdiff. The interdiffs I reviewed, I liked what I saw. ;)

I'm +1 to having "Advisories" as the main terminology here, and differentiating PSAs from SAs. Thanks for sorting that out!

+1 to erring on the side of showing more advisories than we might technically need to show. Way better than not showing something we wished we were showing in an edge case. Fail safe. ;)

+1 to ripping out the complications of partial matches. Your reasoning on being able to do exact matches makes perfect sense at #74.3

+1 to punting on perfection for -dev. Update Manager has always had trouble with -dev, and -dev is just problematic in so many ways. ;) We can always try to improve things later. Better to get something out that handles official releases well, than to delay that while trying to solve every possible -dev thing. #86.1 seems very reasonable for an initial release of this feature.

Re: #86.2: Hah! Guess we can un-postpone #3110223: Add ModuleVersion::getMinorVersion() method. ;) Probably just mark it fixed once this lands.

I'm going to do a final review of the full patch next, but this sounds like it's either RTBC or darn close.

Thanks again!
-Derek

Assigned by accident, sorry.

Mostly everything looks great. The test coverage is extremely thorough. Basically everything that needs to be fixed was fixed.

I know we're almost out of time to get this into 9.1.x, but it's going to be painful to change the tempstore keys later, and some of the test coverage isn't what it says it is. That's why I'm setting NW. Everything else is basically trivial docs stuff or code polishing that could be done in a follow-up. But if we're rerolling anyway, maybe worth fixing now.

Thanks again!
-Derek

1. +++ b/core/modules/update/src/SecurityAdvisories/EmailNotify.php
   @@ -0,0 +1,164 @@
   + * Provides an service to send email notifications for security advisories.
   

   s/an a/

2. +++ b/core/modules/update/src/SecurityAdvisories/EmailNotify.php
   @@ -0,0 +1,164 @@
   +  protected $saFetcher;
   

   Wonder if securityAdvisoryFetcher would be better for this var.

3. +++ b/core/modules/update/src/SecurityAdvisories/EmailNotify.php
   @@ -0,0 +1,164 @@
   +   * @param \Drupal\update\SecurityAdvisories\SecurityAdvisoriesFetcher $psa_fetcher
   ...
   +  public function __construct(MailManagerInterface $mail_manager, SecurityAdvisoriesFetcher $psa_fetcher, ConfigFactoryInterface $config_factory, LanguageManagerInterface $language_manager, StateInterface $state, TimeInterface $time, EntityTypeManagerInterface $entity_type_manager, TranslationInterface $string_translation, LoggerInterface $logger) {
   

   s/psa_fetcher/sa_fetcher/
   (or security_advisories_fetcher if the previous comment happens).

4. +++ b/core/modules/update/src/SecurityAdvisories/EmailNotify.php
   @@ -0,0 +1,164 @@
   +      $messages = $this->saFetcher->getSecurityAdvisoriesMessages();
   

   Don't love the double-plural, but probably not worth fixing. ;)

5. +++ b/core/modules/update/src/SecurityAdvisories/EmailNotify.php
   @@ -0,0 +1,164 @@
   +        'Unable to send notification email because of error retrieving PSA feed: %error',
   

   Is this the "PSA feed" you decided to leave as-is due to existing documentation or something?

6. +++ b/core/modules/update/src/SecurityAdvisories/SecurityAdvisoriesFetcher.php
   @@ -0,0 +1,254 @@
   +   * Constructs a new PsaFetcher object.
   

   s/PsaFetcher/SecurityAdvisoriesFetcher/

7. +++ b/core/modules/update/src/SecurityAdvisories/SecurityAdvisoriesFetcher.php
   @@ -0,0 +1,254 @@
   +    $response = $this->tempStore->get('psa_response');
   ...
   +      $psa_endpoint = $this->config->get('psa.endpoint');
   +      $response = (string) $this->httpClient->get($psa_endpoint)->getBody();
   +      $this->tempStore->setWithExpire('psa_response', $response, $this->config->get('psa.check_frequency'));
   

   Do we want to stop keying all these tempstore things with "psa"?

8. +++ b/core/modules/update/src/SecurityAdvisories/SecurityAdvisoriesFetcher.php
   @@ -0,0 +1,254 @@
   +      return t(
   

   $this->t()

9. +++ b/core/modules/update/src/SecurityAdvisories/SecurityAdvisoriesFetcher.php
   @@ -0,0 +1,254 @@
   +        'Unable to retrieve PSA information from :url.',
   

   s/PSA/security advisory/ ?

10. +++ b/core/modules/update/src/SecurityAdvisories/SecurityAdvisoriesFetcher.php
    @@ -0,0 +1,254 @@
    +              // If the dev version doesn't specify a minor version matching on
    

    Wants a comma between "minor version" and "matching on".

11. +++ b/core/modules/update/src/SecurityAdvisories/SecurityAdvisoriesFetcher.php
    @@ -0,0 +1,254 @@
    +              // If the dev version specifies a minor version then insecure
    +              // version must match on the minor version.
    

    a) s/then insecure/then the insecure/

    b) comma after 1st 'minor version'

12. +++ b/core/modules/update/src/SecurityAdvisories/SecurityAdvisoriesFetcher.php
    @@ -0,0 +1,254 @@
    +        // If existing version is not a dev version then it must match an
    +        // insecure version exactly.
    

    "If the existing version is not a dev version, then it..."

13. +++ b/core/modules/update/src/SecurityAdvisories/SecurityAdvisory.php
    @@ -0,0 +1,207 @@
    +   * The link to the advisory.
    ...
    +  protected $link;
    ...
    +   * @param string $link
    +   *   The link to the advisory.
    

    Should have caught this before, but presumably this is more the URL for the advisory, not a "link" (e.g. not <a href="...">blah</a> Undoubtably that's the key in the feed itself, which is why it's like this. So probably nothing to fix...

14. +++ b/core/modules/update/tests/src/Functional/SecurityAdvisoryTest.php
    @@ -0,0 +1,249 @@
    +    // Alter the 'aaa_update_test' to use the 'aaa_update_project' project name.
    +    // This ensures that for an extension where the 'name' and the 'project'
    +    // properties do not match 'project' is used for matching 'project' in the
    +    // json feed.
    

    A) Thanks, this helps!
    B) "the 'aaa_update_test' module"?
    C) s/json/JSON/

15. +++ b/core/modules/update/tests/src/Functional/SecurityAdvisoryTest.php
    @@ -0,0 +1,249 @@
    +
    
    +++ b/core/modules/update/tests/src/Kernel/SecurityAdvisoriesFetcherTest.php
    @@ -0,0 +1,481 @@
    +    static::assertCount(1, $links);
    ...
    +    static::assertCount(0, $links);
    

    Why static:assertCount() here? We don't do that anywhere else in core.

16. +++ b/core/modules/update/tests/src/Kernel/SecurityAdvisoriesFetcherTest.php
    @@ -0,0 +1,481 @@
    +      'core:exact:non-psa' => [
    +        'feed_item' => [
    +          'is_psa' => 0,
    +          'type' => 'core',
    +          'project' => 'drupal',
    +          'insecure' => [\Drupal::VERSION],
    +        ],
    +      ],
    

    Super minor, but it was a bit jarring to see this among the contrib. Seems easier to make sense of this list if we keep all of contrib together, then all of core.

17. +++ b/core/modules/update/tests/src/Kernel/SecurityAdvisoriesFetcherTest.php
    @@ -0,0 +1,481 @@
    +      'contrib:not-exact:non-psa' => [
    +        'feed_item' => [
    +          'is_psa' => 0,
    +          'type' => 'module',
    +          'project' => 'the_project',
    +          'insecure' => ['1.0'],
    +        ],
    +        'existing_version' => '8.x-1.0',
    +      ],
    ...
    +      'contrib:not-exact:non-psa-reversed' => [
    +        'feed_item' => [
    +          'is_psa' => 0,
    +          'type' => 'module',
    +          'project' => 'the_project',
    +          'insecure' => ['1.0'],
    +        ],
    +        'existing_version' => '8.x-1.0',
    +      ],
    

    The name says "reversed", but these two cases are identical.

18. +++ b/core/modules/update/tests/src/Kernel/SecurityAdvisoriesFetcherTest.php
    @@ -0,0 +1,481 @@
    +      'contrib:semver-non-exact:non-psa' => [
    +        'feed_item' => [
    +          'is_psa' => 0,
    +          'type' => 'module',
    +          'project' => 'the_project',
    +          'insecure' => ['1.0'],
    +        ],
    +        'existing_version' => '1.0.0',
    +      ],
    +      'contrib:semver-major-match:non-psa' => [
    +        'feed_item' => [
    +          'is_psa' => 0,
    +          'type' => 'module',
    +          'project' => 'the_project',
    +          'insecure' => ['1.0'],
    +        ],
    +        'existing_version' => '1.0.0',
    +      ],
    

    These 2 also have different names for the same case.

19. +++ b/core/modules/update/tests/src/Kernel/SecurityAdvisoriesFetcherTest.php
    @@ -0,0 +1,481 @@
    +      'contrib:semver-different-majors:non-psa' => [
    +        'feed_item' => [
    +          'is_psa' => 0,
    +          'type' => 'module',
    +          'project' => 'the_project',
    +          'insecure' => ['7.x-1.0'],
    +        ],
    +        'existing_version' => '8.x-1.0',
    +      ],
    

    Name says "semver" but these are BespokeVer strings.

20. +++ b/core/modules/update/tests/src/Kernel/SecurityAdvisoriesFetcherTest.php
    @@ -0,0 +1,481 @@
    +      'link' => 'http://thesa.com',
    

    I think to be safe we should use example.com for this.

21. +++ b/core/modules/update/update.install
    @@ -177,3 +180,36 @@ function _update_requirement_check($project, $type) {
    +  if (!Drupal::hasService('update.sa_fetcher')) {
    

    Doesn't this want \Drupal:: not just Drupal:: ?

22. +++ b/core/modules/update/update.install
    @@ -177,3 +180,36 @@ function _update_requirement_check($project, $type) {
    +  /** @var \Drupal\update\SecurityAdvisories\SecurityAdvisoriesFetcher $psa */
    
    +++ b/core/modules/update/update.module
    @@ -70,6 +71,27 @@ function update_page_top() {
    +      $psa = \Drupal::service('update.sa_fetcher');
    ...
    +        $messages = $psa->getSecurityAdvisoriesMessages();
    

    Again, s/$psa/$fetcher/ (or something).

23. +++ b/core/modules/update/update.install
    @@ -177,3 +180,36 @@ function _update_requirement_check($project, $type) {
    +  $psa = \Drupal::service('update.sa_fetcher');
    

    "psa" is now a weird local variable name for this.

24. +++ b/core/modules/update/update.module
    @@ -192,6 +217,12 @@ function update_cron() {
    +    /** @var \Drupal\update\SecurityAdvisories\SecurityAdvisoriesFetcher $notify */
    

    s/SecurityAdvisoriesFetcher/whatever the right class is/ ;)

This fixes nearly all of #89:

1. Fixed
2. Went with $securityAdvisoriesFetcher
3. Fixed
4. TBD
5. TBD
6. Fixed
7. TODO - This is the main thing I'd like @tedbow to answer before RTBC.
8. Oh right, it's a static method. Ignore.
9. Fixed (hope that's cool)
10. Fixed
11. Fixed
12. Fixed
13. TBD
14. Fixed
15. Fixed
16. Fixed
17. Fixed
18. Fixed: Added a new case for what that name said, and changed the name for the existing case to not say 'semver'.
19. TBD - not sure how to resolve this. ;) Also not a blocker for commit.
20. Fixed
21. Fixed
22. Fixed
23. Fixed
24. Fixed

Thanks again!
-Derek

@dww thanks for the review and patch!

re #89

4. Changed to getSecurityAdvisoryMessages()
5. Yes this is the documentation about the feed https://www.drupal.org/docs/8/update/automatic-updates#s-public-service-... and also it is actually "psa.json".
   Hopefully we can update that documentation later.
7. Fixed this. I changed the settings group in update.schema.yml to be advisories. and the tempstore key to be advisories_response
9. The fix is good 👍
13. Yes this was like this because the feed uses link but I changed the class to used URL and changed the method to getUrl() because the 1 place we use it in non-test code is
    ':url' => $sa->getUrl(),
19. You said "TBD" for 19) but I think you fixed it. Correct?

    It looks to be the one you didn't fix is 18.

    I changed the 2nd test case there and added a new 1 after that

    'contrib:semver-major-match-not-minor:non-psa' => [
            'feed_item' => [
              'is_psa' => 0,
              'type' => 'module',
              'project' => 'the_project',
              'insecure' => ['1.1.0'],
            ],
            'existing_version' => '1.0.0',
          ],
          'contrib:semver-major-minor-match-not-patch:non-psa' => [
            'feed_item' => [
              'is_psa' => 0,
              'type' => 'module',
              'project' => 'the_project',
              'insecure' => ['1.1.1'],
            ],
            'existing_version' => '1.1.0',
          ],
    

    So making sure we don't do a partial match on semver if the site is NOT using a dev version.

1. Following the example of \Drupal\update\ModuleVersion I am marking the class as @internal and making it final. I am also changing the constructor to private since we have a public factory method createFromArray().

   I think the same argument I used for this pattern in ModuleVersion applies to this class. You can see my argument here #3074993-90: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates and in the next comment @xjm agree and giving more reasons.

   Here is @xjm's reasoning which I think applies directly in this case also

   Normally I'd worry about still not having a policy for how we use private (technically the docs still say we don't use it and the issue's still open) but given that this is a totally new, totally internal, immutable value-object-thing with factory methods (and not injecting services nor of any conceivable use to contrib with the constructor), I think it's actually totally justifiable in this case under a current read of core policy.

2. Personally I think with functionality like this we should do whatever we can to not make this an API. I think the json feed itself should be considered what custom code should interact with if they don't like core's functionality. We already check for the existence of the 2 new services in the container before we use them. Therefore custom code can remove these services and add their own functionality without any errors.

   For this reason I would be in favor of making the other 2 classes in the Drupal\update\SecurityAdvisories namespace also final and @internal. We could always remove final later if a good case was made to do so. We can never make them final though if we don't release them that way.

   I am not willing to hold this issue up for this. I imagine it is not a popular opinion but just wanted to put it out there in case it makes sense to others. I guess technically we could do it in follow-up if it made it in before the code was in a release.

4. 👍
5. 👍
7. 👍🙏
9. 👍
13. 👍
18. "It looks to be the one you didn't fix is 18." -- Indeed, sorry for the mix-up.
19. 🙏 - your fix looks good and makes sense.

1. 👍
2. Hrrrrmm. 😉 I see what you mean. OTOH, final is so final. ;) What's the point of making this a service if folks can't decorate it if they need to?
   

   I think the json feed itself should be considered what custom code should interact with if they don't like core's functionality.

   That's pretty harsh. 😉 This is a lot of code, much of it would be relevant to someone else trying to use this functionality but that needed some additional changes / tweaks. Telling them: "sorry, start from scratch if you don't like what we're doing" isn't very Drupal-esque.

   How about we mark them @internal as a hint, but not actually final? Is that weird?

I also think we could change our minds on this during alpha (if not beta), so I'd rather not hold this up for commit. If you want, we can open a follow-up to continue this discussion.

I've carefully reviewed the latest interdiff, and very closely reviewed the full patch twice. Try as I might, I'm out of things to complain about. 😉 Bot's happy. Lots of great new test coverage. No CS violations. Summary is accurate with screenshots, API changes is correct, etc. It was missing a release note, but I added one.

This is important functionality to help improve site security. Let's get this in. RTBC 🎉

Thanks again, @tedbow, @beautifulmind, @heddn, @drumm, @xjm and everyone else who worked on getting this to this point!

Cheers,
-Derek

p.s. Adding #3110223: Add ModuleVersion::getMinorVersion() method as related. It's possible there's some useful test coverage in that patch we can still harvest even after this lands. Of course, not a blocker or urgent. ;)

@dww thanks for the review again!

@xjm asked me to put the "Needs release manager review"

@Pasqualle, only committers should move minor-only issues back to 9.1.x after the release of alpha1 (and generally only after commit). Thanks!

Reference: https://www.drupal.org/about/core/policies/core-change-policies/allowed-...

For what it's worth, we probably will backport this issue, but following the issue deadline this decision is made only once the patch has successfully been committed to 9.2.x. So setting the branch accordingly.

Please do not change the branch elsewhere for minor-only issues; if they don't get backported during the alpha at committer discretion, what you're doing is mixing issues that were specifically targeted against a minor branch in with the patch queue that 9.1.x will shortly become.

One thing that occurred to me since we trimmed down the email message to this for readability:

SUBJECT: An urgent security announcement requires your attention for Drupal

An important security announcement is available for your Drupal site. You should 
read the announcement immediately and follow its instructions.

Public service announcements:

  * Critical Release - SA-2019-02-19 [1]

To see all public service announcements, visit
https://www.drupal.org/security/psa [2].


[1] https://www.drupal.org/sa-2019-02-19
[2] https://www.drupal.org/security/psa

Among other things, we deliberately took out this bit:

Your site is currently configured to send these emails when a security update will be made available soon. To change how you are notified, you may <a href="{{ settings_link }}">configure email notifications</a>.

Since then, I've been wondering if this violates some user rights best practice of explaining to someone how they can unsubscribe for an email notification. Going to ping the sec team to see if they have thoughts on whether we need to include such a sentence here. (If we do have to add it back, we could just add back the last sentence rather than both of them.)

Per @webchick, we do indeed need a link telling the email recipient how to change the notifications. @webchick referenced the CAN-SPAM, act, and I believe there are other legal precedents as well.

I think just the second sentence will do:

 To change how you are notified, you may <a href="{{ settings_link }}">configure email notifications</a>.

Updated email

Interdiff looks great.
All 3 templates updated, and test coverage expanded to match. Tests are all passing.
No CS violations.
Back to RTBC.

Thanks!
-Derek

Reviewed down to the start of the tests. Mostly nitpicks but one critical question WRT dev versions in point 12.

1. +++ b/core/modules/update/config/schema/update.schema.yml
   @@ -40,3 +40,13 @@ update.settings:
   +    advisories:
   +      type: mapping
   +      label: 'Security advisory settings'
   +      mapping:
   +        endpoint:
   +          type: string
   +          label: 'Endpoint URI for security advisories'
   +        check_frequency:
   +          type: integer
   +          label: 'Frequency to check for security advisories, defaults to 12 hours'
   

   Didn't we discuss using an existing configuration setting for this check frequency?

2. +++ b/core/modules/update/src/ModuleVersion.php
   @@ -73,7 +86,7 @@ public static function createFromVersionString($version_string) {
   -    return new static($major_version, $version_extra);
   +    return new static($major_version, $minor_version, $version_extra);
      }
    
      /**
   @@ -81,11 +94,14 @@ public static function createFromVersionString($version_string) {
   
   @@ -81,11 +94,14 @@ public static function createFromVersionString($version_string) {
       *
       * @param string $major_version
       *   The major version.
   +   * @param string|null $minor_version
   +   *   The minor version.
       * @param string|null $version_extra
       *   The extra version string.
       */
   -  private function __construct($major_version, $version_extra) {
   +  private function __construct(string $major_version, $minor_version, $version_extra) {
        $this->majorVersion = $major_version;
   +    $this->minorVersion = $minor_version;
        $this->versionExtra = $version_extra;
      }
   

   I was going to say that this is a BC break, but we did make the constructor private.

3. +++ b/core/modules/update/src/SecurityAdvisories/EmailNotify.php
   @@ -0,0 +1,164 @@
   +   * EmailNotify constructor.
   

   "Constructs a"

4. +++ b/core/modules/update/src/SecurityAdvisories/EmailNotify.php
   @@ -0,0 +1,164 @@
   +   * Send notification when security advisories are available.
   

   "Sends notifications"

5. +++ b/core/modules/update/src/SecurityAdvisories/EmailNotify.php
   @@ -0,0 +1,164 @@
   +  public function send(): void {
   

   Missing space before the colon.

6. +++ b/core/modules/update/src/SecurityAdvisories/EmailNotify.php
   @@ -0,0 +1,164 @@
   +        'Unable to send notification email because of error retrieving PSA feed: %error',
   

   Since this can end up as a user-facing string, let's use a complete sentence with articles:

   Unable to send a PSA notification email because of an error retrieving the PSA feed.

7. +++ b/core/modules/update/src/SecurityAdvisories/SecurityAdvisoriesFetcher.php
   @@ -0,0 +1,254 @@
   +      $this->tempStore->setWithExpire('advisories_response', $response, $this->config->get('advisories.check_frequency'));
   

   Why a tempstore?

8. +++ b/core/modules/update/src/SecurityAdvisories/SecurityAdvisoriesFetcher.php
   @@ -0,0 +1,254 @@
   +        if ($project_type !== 'core' && (!isset($this->extensionLists[$project_type]) || !$this->getProjectInfo($sa))) {
   +          continue;
   +        }
   

   Hm, code comment here would be good as it's not immediately obvious why we skip it under these conditions.

9. +++ b/core/modules/update/src/SecurityAdvisories/SecurityAdvisoriesFetcher.php
   @@ -0,0 +1,254 @@
   +  public static function getErrorMessageFromException(\Exception $exception, bool $throw_unexpected_exceptions = TRUE) {
   

   Noe to self: check for where this bool flag is set.

10. +++ b/core/modules/update/src/SecurityAdvisories/SecurityAdvisoriesFetcher.php
    @@ -0,0 +1,254 @@
    +   *   TRUE if the security advisory matches the existing version of the
    +   *   project, otherwise FALSE.
    

    "or FALSE otherwise."

11. +++ b/core/modules/update/src/SecurityAdvisories/SecurityAdvisoriesFetcher.php
    @@ -0,0 +1,254 @@
    +          catch (\UnexpectedValueException $exception) {
    +            // Version numbers that start with core prefix besides '8.x-' are
    +            // expected in $insecure_versions but will never match and will
    +            // thrown an exception.
    

    Note to self: Check for test coverage of all three cases here.

12. +++ b/core/modules/update/src/SecurityAdvisories/SecurityAdvisoriesFetcher.php
    @@ -0,0 +1,254 @@
    +          if ($existing_project_version->getMajorVersion() === $insecure_project_version->getMajorVersion()) {
    +            if ($existing_project_version->getMinorVersion() === NULL) {
    +              // If the dev version doesn't specify a minor version, matching on
    +              // the major version alone is considered a match.
    +              return TRUE;
    +            }
    +            if ($existing_project_version->getMinorVersion() === $insecure_project_version->getMinorVersion()) {
    +              // If the dev version specifies a minor version, then the insecure
    +              // version must match on the minor version.
    +              return TRUE;
    

    Wait. With a dev version, we have no indication of how recent it is. It could be from yesterday or five years ago.

13. +++ b/core/modules/update/src/SecurityAdvisories/SecurityAdvisoriesFetcher.php
    @@ -0,0 +1,254 @@
    +   * @return array
    +   *   The project information if the project exists, otherwise an empty array.
    

    Can we be more specific than array? And where is the array structure canonically defined?

    Also: "or otherwise".

14. +++ b/core/modules/update/src/SecurityAdvisories/SecurityAdvisoriesFetcher.php
    @@ -0,0 +1,254 @@
    +  protected function getProjectInfo(SecurityAdvisory $sa): array {
    

    Missing space before the colon again. We should check for or file a coder rule followup for this.

15. +++ b/core/modules/update/src/SecurityAdvisories/SecurityAdvisory.php
    @@ -0,0 +1,209 @@
    + * These come from the PSA feed on drupal.org.
    

    We have a content standard that "Drupal.org" should be capitalized.

16. +++ b/core/modules/update/src/SecurityAdvisories/SecurityAdvisory.php
    @@ -0,0 +1,209 @@
    +   *   The versions of the project that are currently insecure. For PSAs this
    +   *   list does include versions that will be marked as insecure when the new
    +   *   security release is published.
    

    Why "does"?

17. +++ b/core/modules/update/src/SecurityAdvisories/SecurityAdvisory.php
    @@ -0,0 +1,209 @@
    +  public function getTitle(): string {
    ...
    +  public function getProject(): string {
    ...
    +  public function getProjectType(): string {
    ...
    +   */
    ...
    +  public function getInsecureVersions(): array {
    ...
    +  public function getUrl(): string {
    

    Spaces before the colons.

18. +++ b/core/modules/update/templates/update-advisory-notify.html.twig
    @@ -0,0 +1,34 @@
    +    An important security announcement is available for your Drupal site. You should read the announcement immediately and follow its instructions.
    +  {%- plural advisories -%}
    +    Important security announcements are available for your Drupal site. You should read the announcements immediately and follow their instructions.
    

    Should we omit the word "Drupal" here? Going back to the content standard of avoiding using the word "Drupal" in user-facing content since it can be branded as something else. Edit: Just checked and the existing email notifications about insecure sites do say "Drupal" already. Dunno if we should file a followup about that or let it be. Maybe just let it be since it's been this way forever.

+++ b/core/modules/update/config/schema/update.schema.yml
@@ -40,3 +40,13 @@ update.settings:
+          label: 'Frequency to check for security advisories, defaults to 12 hours'

Also: comma splice. Should be two sentences.

With a dev version, we have no indication of how recent it is. It could be from yesterday or five years ago.

Right. So the patch errs on the side of displaying the SA. There's a chance you have a dev snapshot that already includes the fix, but we can't determine that. However, it shouldn't be all that likely that you have a dev snapshot that includes the fix. The most common reason to be on a dev snapshot is that you need to be ahead of the last tag that was made on that branch. But it that was your situation prior to the SA, then once there's an SA, there's either a new tag on that branch, or that branch is no longer supported and you need to update to the branch that has the security fix and there's now a newly released tag on that one. Ok, but what if a day or a week after that SA, a non-security bug fix gets committed on that branch, and your site needs it? This is the situation in which you'd now be on a dev snapshot and seeing the old SA again, even though you already have the fix. While not ideal, is it acceptable, considering that if we don't show the SA for dev snapshots, then the more common case of someone being on an old dev snapshot would not see the SA?

I have made the following changes in this patch:
103.3
103.4
103.5
103.6
103.10
103.13.2
103.14
103.15
103.16
103.17
and #104

Remaining changes are:
103.1
103.2
103.7
103.8
103.9
103.11
103.12
103.13.1

@xjm thanks for the review and @ayushmishra206 thanks for addressing many of the items!

re #103

1. I remember discussing that and the use of the existing email setting but I thin the conclusion was that the existing check frequency can not be used because you can set it to "weekly" with many of these items being in the feed for a week or less then a check once a week for a PSA for an upcoming release within a week would be pretty much meaning less for many sites.
2. Yeah this was one main reasons to make the constructor private and the reason to make the new \Drupal\update\SecurityAdvisories\SecurityAdvisory::__construct() also private
3. #106 addressed this but needs to change from "Constructs a EmailNotify constructor" to "Constructs a EmailNotify object"
4. 👍
5. 👍
6. 👍
7. To make this the same as UpdateProcessor and UpdateManager in the Update module. @xjm see your comment in #57.13
   

   Interesting. This seems like it should be stored in state rather than cache? Otherwise a cache clear blows it away and resets it.

   However, we should primarily mimic whichever pattern the update module already does when checking for updates.

8. Added a comment on why we skip here.
9. Flag is set in the catch in \Drupal\update\SecurityAdvisories\EmailNotify::send(). I think my intention was to not to have an exception that we were not expecting stop cron. Now looking at \Drupal\Core\Cron::invokeCronHandlers() exceptions are already caught and handled. So removing this $throw_unexpected_exceptions
10. 👍
11. See test cases in \Drupal\Tests\update\Kernel\SecurityAdvisoriesFetcherTest()
12. See @effulgentsia comment in #105
13. Changed to mixed[]. This is from the info.yml files but augmented in \Drupal\Core\Extension\ExtensionList::getList() or where this function overridden and also passed through hook_info_alter()
    I don't believe we have a canonical place where this structure is documented. Update the @return comment
14. 👍
15. 👍
16. This should have been "does not". fixed
17. There were some more of these but now I am unsure if having the space before the colon is actually correct.
    The example in the proposed change in #2928856: Adopt the PSR-12 standard for PHP return types suggest not to have a space before colon. grepping in /core for "function.*\):" with no space finds over 100 results and "function .*\) :" with the space only finds 27.

    Not changing any more for now

18. I would say to let it be because I think it is highly likely that people may have set up email filters for this exact text in the existing emails to flag these emails. Also I would say if we don't already have an issue where people have asked to change this because it is causing confusion when a site is branded as something else then changing it will probably cause more problems than it is worth. I didn't check how to override the existing emails but since this is in a twig template it is easily changeable for any sites that want to.

I remember discussing that and the use of the existing email setting but I thin the conclusion was that the existing check frequency can not be used because you can set it to "weekly" with many of these items being in the feed for a week or less then a check once a week for a PSA for an upcoming release within a week would be pretty much meaning less for many sites.

Makes sense.

There were some more of these but now I am unsure if having the space before the colon is actually correct.
The example in the proposed change in #2928856: Adopt the PSR-12 standard for PHP7 return types suggest not to have a space before colon. grepping in /core for "function.*\):" with no space finds over 100 results and "function .*\) :" with the space only finds 27.

In that case, can we be consistent the other way, and not have the space before the colon anywhere? This sort of violates core's standard of having a space on either side of an operator, but we should at least be consistent within the patch.

#2928856: Adopt the PSR-12 standard for PHP return types doesn't actually specify whether there should be a space before the colon or not. It only says there does need to be a space after it.