Add tests for when a single supported branch has 2 security updates that are both not insecure

I don't think this is the right approach. Sometimes we will have joint releases, like 8.2.8 and 8.3.1. So in that case 8.3.1 is equivalent from a security perspective. A couple months later 8.3.4 ccomes out with a new security fix that is not backprted to 8.2.x, so 8.3.1 is then insecure.

I think what we should actually be showing is just the latest security release on that branch and the latest security release on subsequent branches. Or something.

There is documentation in https://api.drupal.org/api/drupal/core%21modules%21update%21update.compa... that explains the justification for the current behavior:

Finally, and most importantly, the release history continues to be scanned until the currently installed release is reached, searching for anything marked as a security update. If any security updates have been found between the recommended release and the installed version, all of the releases that included a security fix are recorded so that the site administrator can be warned their site is insecure, and links pointing to the release notes for each security update can be included (which, in turn, will link to the official security announcements for each vulnerability).

Personally I agree with that reasoning, so I don't think all the non-up-to-date security releases should be removed completely.

That said, the old ones definitely don't need to take up so much screen space (with download links and the whole rigmarole). I could definitely imagine it being streamlined down to a single line, something like "Previous security releases: X, Y, Z..." where each one is just a simple link to the release notes.

That said, the old ones definitely don't need to take up so much screen space (with download links and the whole rigmarole). I could definitely imagine it being streamlined down to a single line, something like "Previous security releases: X, Y, Z..." where each one is just a simple link to the release notes.

Good point, this makes sense. Retitling to that scope. Also going to add an annotated screenshot so that I don't continue to confuse this with all the things.

Actually, retitling to the bug rather than a proposed resolutiton

So hm, OTOH. For non-security releases, we don't list all the releases. We only list the latest one. The site owner probably still needs to know the things in the intervening release notes. If you're on 8.3.7 and planning to update to 8.5.1, you probably should review both the 8.4.0 and 8.5.0 release notes.

This is probably another thing that we should have design/usability work for, rather than three backend developers trying to decide what to do with a UI. :)

Anyway here's the screenshot so I understand/rememebr which issue this is, and separating the problem statement from the proposed resolution in the IS.

So yeah, I don't think we should link all the releases because we don't want the user to choose one of the releases and install them. We want them to install the latest security release.

Maybe what we should do instead is add a one-liner to our security release notes template that is something like:

See all past security advisories for this project.

Tagging for design/UX feedback.

Perhaps a wall of scary red will encourage upgrading? I just don’t understand how a site owner could see that and not jump to action.

That said, don’t we want to highlight the security release higher than the installed version that is on a supported branch?

Perhaps a wall of scary red will encourage upgrading? I just don’t understand how a site owner could see that and not jump to action.

That was indeed the intention of the original design + implementation. ;)

So this issue is resolved now. Since @drumm marked old releases "insecure" for #2804155: If the next minor version of core has a security release, status still says "Security update required!" even if the site is on an equivalent, secure release already, only the latest security update is shown. This can be tested by installing Drupal 8.1.0 and checking the update status report.

We should use this issue to add test coverage for it.

I think a way to address this:

If any security updates have been found between the recommended release and the installed version, all of the releases that included a security fix are recorded so that the site administrator can be warned their site is insecure, and links pointing to the release notes for each security update can be included (which, in turn, will link to the official security announcements for each vulnerability).

Would be to instead just add a link to https://www.drupal.org/security?

I think the link in #12 could be helpful, although it won't give the reader clear feedback on which of those advisories applied to their own site.

It seems like the actual goal for #18 is to have a place that the reader can see all of the advisories that apply to their site, in one place. But I feel like that's outside of the scope of this issue, and could be a follow up. Only showing the latest release is much better for the user than showing a wall of releases with the possibility that the user will install the wrong one.

In #2990511: Add comprehensive test coverage for update status for security releases @tedbow identified that the site can show older security releases than the site's currently installed version, which is definitely not the right behavior. @tedbow filed #2992631: Update report incorrectly recommends security releases for old minors when a security update is needed and a secure version of the old minor is also available for that, though it will probably end up being fixed by whatever's left to fix here following the d.o metadata change. @tedbow has a couple test cases for instances that still show multiple releases and will attach them here for now.

1. re the solution in #3

   +++ b/core/modules/update/update.report.inc
   @@ -168,12 +168,18 @@ function template_preprocess_update_project_status(&$variables) {
   -          $versions_inner[] = [
   -            '#theme' => 'update_version',
   -            '#version' => $security_update,
   -            '#title' => t('Security update:'),
   -            '#attributes' => ['class' => $security_class],
   -          ];
   +          // Show only the latest security update for the series. For core, the
   +          // series is the minor version (8.1.x vs. 8.2.x). For other projects,
   +          // this is the major version, (8.x-1.x vs. 8.x-2.x).
   +          $key = 'sec-' . $security_update[$project['project_type'] === 'core' ? 'version_minor' : 'version_major'];
   +          if (!isset($versions_inner[$key]) || version_compare($security_update['version'], $versions_inner[$key]['#version']['version']) === 1) {
   

   I don't think we should be solving this on the return template level.

   I think update_calculate_project_update_status() tries to solve this and should be the authority on what security updates are applicable.

2. as @xjm said in #21 some the metadata changes coming from drupal.org update xmls should fix a lot of this. Most previous security releases will have the Insecure tag because they contain security hole that was found in later security updates so they will be marked as insecure. So update_calculate_project_update_status() will no longer return them.
3. This did not fix the case though of when you are on insecure version and Drupal was still showing you security releases that were themselves secure(i,e not tagged with Insecure) but they would be a downgrade from the sites current version.

   This is because in update_calculate_project_update_status() there is code to try stop looping through releases when we find the current version of project.

    // Stop searching once we hit the currently installed version.
    if ($project_data['existing_version'] === $version) {
      break;
    }
   

   This seems correct and should avoid showing any releases before the current version. But if we look a little above this

   // Otherwise, ignore unpublished, insecure, or unsupported releases.
       if ($release['status'] == 'unpublished' ||
           (isset($release['terms']['Release type']) &&
            (in_array('Insecure', $release['terms']['Release type']) ||
             in_array('Unsupported', $release['terms']['Release type'])))) {
         continue;
       }
   

   If the current release is insecure then we will continue; looping through releases and not reach the break; above to which should stop us from looping through prior releases.

   I will upload a fix for just this.

4. I will also upload a patch with the test cases @xjm mentioned in #21 that will mostly pass with this fix.
5. There is one test that will still fail.
   

   '8.x-3.0-beta1, 8.x-1.2 8.x-2.2' => [
        'module_patch_version' => '8.x-3.0-beta1',
        'expected_security_releases' => ['8.x-2.2'],
        'expected_update_message_type' => static::SECURITY_UPDATE_REQUIRED,
        'fixture' => 'sec.8.x-1.2_8.x-2.2',
    ],
   

   I am not sure what the correct behavior is here.

   In the fixture used for the test 8.x-3.0-beta1, the installed version, is tagged Insecure. 8.x-3.0-beta2 is not Insecure but is also not a Security update. This because pre-releases besides RC releases cannot marked Security updates.

   I think @xjm argued we should show the previous Security update 8.x-2.2 because the site is on an Insecure version and this the only available Security update.

   The problem of course is that this would downgrade. The fix above would have to be extended to show at least 1 previous security release. Also if 8.x-3.0-beta1 contain any non downgrade changes such as entity field additions or other table changes you site would likely break.

   The fix above recommend an upgrade 8.x-3.0-beta2 but this would not be label as a Security upgrade.

We determined previously that it's hard to resolve the other UX issues with the status report if this one isn't also solved, so explicitly adding it to the roadmap for the security coverage initiative.

We should also re-test what happens with this issue now that Drupal.org has changed how insecure releases are handled: Does the wall of terror persist?

1. Locally testing 9.2.0 should determine if this problem still exists. I test and this is what I see

   

   I think the problem is fixed but we should have test to confirm this doesn't happen again.

2. we still have to decide what to do with #22.5

   in that case should we recommend 8.x-3.0-beta2 which is not security release but also not insecure

   or should we recommend 8.x-2.2 which is security release but it also a downgrade.

3. We have a todo in \Drupal\Tests\update\Functional\UpdateContribTest::securityUpdateAvailabilityProvider() to add the missing test case mention above.

   We should start a new merge request with that test case which will now fail.

4. Then we should look at changes if it is determined that 8.x-2.2 should be recommended.

   This would mean we would have to change update_calculate_project_update_status() to look for a security release that is less than current version

Since the original title of this issue no longer really applies, I think we should probably re-title this and update the issue summary. Tagging for those.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Issue Summary update and title change

#31
1. Already fixed.
2. Added test case and removed @todo in UpdateContribTest.
3. The MR for this is MR1615
4. Check if these results are what is wanted:

Since 1. was her the part affecting usability and was fixed already and the remaining task for this ticket is the test, I'm removing "usability" and needs usability review" tags. If we need to look at the usability aspects of the message (such as order of the recommendations, security updates on each supported minor etc.) it should prbably be a new issue.

Attaching an example screnshot for 10.1.1 message for reference.

Rebased and fixed phpcs issue
Hiding patches and old MR (but then un-hid_

Will remove tests tag as test branch is showing failure.

But can we update the MR 1594 for 11.x and include tests too please.

Unsure about the retitle and IS rewrite here. I am not sure the issue was totally fixed, just that it was mitigated by a recent change to d.o. The issue is still partly present AFAIK.

According to #32.1 this original problem is fixed. tedbow states that they tested on 9.2 and "I think the problem is fixed but we should have test to confirm this doesn't happen again".

As this was tagged, I had this queued for UX review meeting on May 10 but while preparing, I couldn't reproduce the original usability issue as it was already fixed as mentioned in #32. We didn't therefore do a formal review/recommendations and the related discussion is not included in the meeting recording.