There's some important information about which releases get security advisories over in the handbook but that's not in a contextually relevant location so most people don't know about it.

UI changes

Shield icon from #2721227: Design a security shield icon
Mockup
Link to https://www.drupal.org/security-advisory-policy

Mockup as it might appear on the download table:

A more detailed mock up of how this might appear in concert with several other suggested changes is in comment #82

Implementation

Deployment

  • Update project, bluecheese, drupalorg
  • drush vset project_release_downloads_view drupalorg_project_downloads
  • Clear download table cache

Comments

mr.baileys’s picture

FileSize
52.34 KB

I agree that there should be a visible disclaimer warning users of the implications of running non-stable releases. I'd include at least some basic information on the project page itself (as opposed to just linking to the handbook page). Something like:

WARNING: non-stable releases (-dev, -alpha, -beta, -rc) should not be used on production sites. Read the security announcements and process policy for information on which releases get Security Advisories.

Download warning

greggles’s picture

That looks beautiful to me. I'm not sure we should say "WARNING" quite like that. Especially if a module has only stable releases there's no need to show this text. But even if it has just dev releases the words "WARNING" feel a little over the top.

We also have to be careful here - the goal is to make end users aware of the policies around the software they use. But if we make this too rude then module maintainers may make a 1.0 release right away which we've seen is not a great thing to promote.

kiamlaluno’s picture

We also have to be careful here - the goal is to make end users aware of the policies around the software they use. But if we make this too rude then module maintainers may make a 1.0 release right away which we've seen is not a great thing to promote.

It's true that the message should not sound too rude, but the risk maintainers create a 1.0 (or 1.1) release is already present; as any alpha, beta or release candidate versions are not listed in the project page when there is already an official release that is not a alpha, beta or release candidate, a maintainer could create version 1.1 (or 1.2) just to make the more recent version of the module visible in the project page.
IMO, there is nothing rude on the warning message as suggested by mr.baileys; we can remove the WARNING: at the beginning of the sentence, but the rest of the sentence is perfectly fine for me. Plus, it would avoid maintainers have to explicitly report in all my project pages that alpha, beta or release candidate versions are not suggested for production sites.

dww’s picture

Issue tags: +Needs usability review

FYI: theme_project_release_project_download_table() is your friend. See contributions/modules/project/release/project_release.module. bluebeach is not yet overriding this, so that's all you'd need to do to insert text right there.

In general, I'm in favor of a notice like this, but I'm also keenly aware that the UI/UX for project nodes is a contentious subject. So, we should get some UX review before this is actually implemented and rolled out live.

mgifford’s picture

I think it might make sense to place the the text of the notice below the Development Releases heading. It will be accessible if it is in text, but will be better if it is closer to the content that is potentially a concern.

I'd suggest the following additions:

* an ID for <h4 id="development-releases"> so that we can link to that heading.
* provide a link to the notice closer to the link See notice above

<div class="download-table download-table-error"><h4 id="development-releases">Development releases</h4>
<div class="view view-project-release-download-table view-id-project_release_download_table view-display-id-attachment_3 view-dom-id-3">
  
  <strong>NOTICE:</strong> Non-stable releases (-dev, -alpha, -beta, -rc) should not be used on production sites. Read the security announcements and process policy for information on which releases get <a href="http://drupal.org/node/475848">Security Advisories</a>. 
  
      <div class="view-content">
      <table class="views-table">

...

  <tbody>
          <tr class="odd views-row-first views-row-last">
                  <td class="views-field views-field-version">
            <a href="/node/697140">6.x-2.x-dev</a>          </td>
                  <td class="views-field views-field-files">
            <a href="http://ftp.drupal.org/files/projects/connect-6.x-2.x-dev.tar.gz">Download <span class="filesize">(72.81 KB)</span></a> -  <a href="#development-releases">See notice above</a>      </td>

                  <td class="views-field views-field-file-timestamp">
            2010-Apr-24          </td>
                  <td class="views-field views-field-view-node">
            <a href="/node/697140">Notes</a>          </td>
              </tr>
      </tbody>
</table>
    </div>

mgifford’s picture

@greggles just brought up RC's in IRC which wouldn't fit into this example. I'd like to see some way of indicating an ID to link to directly:

<p id="advisory-notice"><strong>NOTICE:</strong> Non-stable releases (-dev, -alpha, -beta, -rc) should not be used on production sites. Read the security announcements and process policy for information on which releases get <a href="http://drupal.org/node/475848">Security Advisories</a>.</p>

As well as a link near the download link to point folks to the notice.

<a href="http://ftp.drupal.org/files/projects/connect-6.x-2.x-dev.tar.gz">Download <span class="filesize">(72.81 KB)</span></a> -  <a href="#advisory-notice">See notice above</a>

A simple text link is probably best. There may be accessibility issues if you wanted to have a dialog window jump up to warn people when they are clicking on the link.

yoroy’s picture

Of course security is the Most Important Thing In The World etc, but is there a specific real-world reason/trend/issue or a pre-emptive just in case? :)
In the scheme of this page, does it really need its own dedicated new spot or can we add it to one of the existing lists of links?

My idea for the text would focus on stressing the positives instead of warning about the negatives:
"Only issues with stable releases (Y.x-Z.0 or higher) in the [a]supported major version branches get security advisories.[/a]"

securityadvisories

greggles’s picture

I agree with yoroy's suggestion. It's similarly important to the other items in the resources.

For some insight into why this is a problem - basically nobody knows that this is the policy :( Even within the security team there was confusion. I like the idea of taking a small step here at first and seeing if that helps enough.

dww’s picture

If we want to add this to the resources section, then you'll want drupalorg.module after all:

function drupalorg_project_page_link_alter(&$links, $node) {
  // Link to security handbook page.
  $links['development']['links']['report_security_issue'] = l(t('Report a security issue'), 'security-team');
}

So, for example, you'll want to add something like:

  $links['resources']['links']['sa_policy'] = l(t('Releases that get security advisories'), '/node/475848');
greggles’s picture

Status: Active » Needs review

Awesome - thanks for the tips, dww.

Anyone else have input on this before we put it live?

greggles’s picture

FileSize
923 bytes
kiamlaluno’s picture

Status: Needs review » Reviewed & tested by the community
dww’s picture

Status: Reviewed & tested by the community » Needs work

Hate to do this, but since some folks completely reorganized the file layout and directory structure of drupalorg just *after* they branched for DRUPAL-6--1, we need completely different patches for D6--1 (production) and HEAD (d.o redesign). :(

The attached is for HEAD. But, to see this live, we need a similar patch against modules/drupalorg/drupalorg.module itself.

I also wouldn't mind a bit more review and agreement on the textual change itself (and do we really want/need to make project nodes a bit more wall-o-text-ish for this)?

greggles’s picture

The reason for the change is in the OP and #8 - it's a policy that amazingly few people know about and yet it could change your decision about whether to download a module or not.

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
991 bytes
892 bytes

Rerolled greggles' patch from #11 against both HEAD and D6--1 and included a comment. I do think that by moving the link to the resources section (basically putting it in the list of RTFM links nobody reads) it's going to be less effective than originally envisioned, but I understand the UI concern of slapping a big warning label near the download table. And putting it in the resource section is a step forward compared to the current situation.
Not sure about the current wording of the link:

Releases that get security advisories

This reads to me as "click here to see a list of releases for this project that might get a security advisory", especially given the fact that the other links in this section pertain to an individual project. Could we re-word it a bit so it's clear that it's a general policy? Also, it should probably start with a verb, since the 3 links above it do (Read ..., View ...). Something simple like:

See security advisory policy

mr.baileys’s picture

FileSize
716 bytes

Rerolled post-redesign patch.

David_Rothstein’s picture

For what it's worth, I thought of this idea independently (without realizing this issue existed) and the text I came up with was this:

This release is not recommended for use on production sites. It is still under development, and not supported by the Drupal security team.

I think there's some value in having a direct statement like that, because people need clear guidance when deciding whether or not they should use a module on their site.

However, starting off with a simple link seems like a reasonable first step to try. For the language, I agree a verb is needed. Why not the original text that @greggles proposed, then? "Learn which releases get security advisories". Or maybe, "Learn which releases are supported by the Drupal security team"? ("Security advisories" may not be a familiar term to everyone who views this page.)

dww’s picture

I like the direction of "Learn which releases are supported by the Drupal security team" but "supported" can send the wrong message. I bet lots of people will expect that means it's been reviewed and "endorsed" or "approved" by the Security Team. I think we need something like:

- handled
- updated
- overseen
- managed
- cared for
- coordinated

So, perhaps:

"Learn which releases are handled by the Drupal Security Team"

greggles’s picture

Status: Needs review » Needs work

"handle" still feels a bit too active for me.

The big public facing difference is that it gets an SA and our best effort at confidential reporting. Modules without a stable release can still get the "security update" tag which makes update.module notice them and puts them in some rss feeds.

That said, I won't block progress on this for finding the perfect word. Among the alternatives dww proposed and that I can think of "handle" still seems best.

yoroy’s picture

monitored by?

greggles’s picture

Monitor still feels like an active verb. If you monitor a dam then you are checking on it to make sure it isn't leaking. That's not analogous to the security team's actions.

Handled still feels best to me among these alternatives.

mr.baileys’s picture

"Covered by ..."?

tvn’s picture

Status: Needs work » Closed (won't fix)

Closing old issues. Please re-open if needed.

David_Rothstein’s picture

Issue summary: View changes
Status: Closed (won't fix) » Needs work

This is still extremely relevant.

It has also come up recently in #2453587: [policy, no patch] Changes to the project application review process, where the proposal seems to be to add a more dire warning in the case of a project that does not get security advisories. Though we have to be careful about going too far with that, as discussed in this issue.

David_Rothstein’s picture

Project: Drupal.org webmasters » Drupal.org customizations
Version: » 7.x-3.x-dev
Component: Textual improvements » Code

Moving to the correct queue (I think)?

David_Rothstein’s picture

Component: Code » User interface
davidhernandez’s picture

greggles’s picture

Thanks for reopening this!

I like the word coordinated :)

Anyone else have proposals for words/designs that will work?

Bojhan’s picture

Issue tags: -Needs usability review

I am not really sure about this at all, what is the goal here? From reading the handbook, I am not even sure what we are supposed to be telling here.

David_Rothstein’s picture

Anyone else have proposals for words/designs that will work?

In retrospect I still think the original wording or something similar to it is pretty good:

"Learn which releases get security advisories"

Or maybe:

"Learn which releases are covered by the security advisory policy"

YesCT’s picture

Issue tags: +security
greggles’s picture

Issue tags: +Needs usability review

@Bojahn, the goal is to let people know whether a module is or isn't covered by a security release. The green/yellow/red system gives some broad sense, but leaves a lot of room for misinterpretation (and doesn't actually align accurately to what is covered or not from a security perspective). If you're still unsure of the goals please ping one of the participants in irc? Thanks.

Bojhan’s picture

Thanks for clarifying.

I am not sure about the overall approach, this text can easily be missed as users are essentially extremely task focused - they will try to find the latest download they can click. While I understand that this is the minimal effort to do, it will likely also have a minimal impact on achieving your goals.

To achieve your goal, I suggest we do a small redesign exercise - where we for example; hide this from initial view, place it on a different page, place them further apart. Sadly the module page has received little love in the past few years, I actually have a redesign laying around.

But sadly I don't really have the time to take this further. Is this something the d.o team could pick up?

joshuami’s picture

Issue summary: View changes
Issue tags: +d.o find and select projects
FileSize
112.36 KB

I think we can make the argument for this fitting into the find and select projects initiative, so I'm marking it as such.

Perhaps we should also tackle this issue from the edit release UI as well. (See attached.) It was mentioned above that the security team and maintainers do not know the rules around security review, this would go a long way towards helping maintainers understand when they should actually use modifiers like -alpha, -beta, -rc.

Addition of text to edit release view

jthorson’s picture

Ooops ... (re)found this after opening #2691081: Add SA Coverage notifications to project pages. Here's a mock we came up with at MidCamp. I would suggest something like this combined with the suggestion to only show 'stable' releases as green in the 'Recommended Releases' section of the download table, with non-stable releases shown in yellow.

jthorson’s picture

Status: Needs work » Needs review

Needs review for feedback on mockups

yoroy’s picture

Red is for errors, these are warnings at most, but really just information, no? Keep in mind what kind of message this would send out to people seeing this. Beware of crying wolf too often, we don’t want to train people to just ignore all the red boxes.

It's all very alarming looking and seems to go completely overboard. Maybe go back and limit this to the initial consideration: tell which releases are covered by security advisories and don't yell about the ones that aren’t. (big red boxes == yelling, visually). And two paragraphs of explanations is documentation, not a message :-)

And in contrast to the strong disclaimers for the releases not covered, the "…are considered covered" is a rather weak endorsement for the releases that are.

Only local images are allowed.

Why nog put the checkmark directly next to the release itself and add a short explanation + link to the full story below the downloads table. The downloads are already pushed down the page by all the info that comes first, lets not push it further down. We do want people to download modules! :-)

yoroy’s picture

greggles’s picture

@yoroy I agree with your comment in #37, but the image somehow disappeared. Can you upload again?

yoroy’s picture

Thanks @greggles, done. It's a rough sketch only. We could look into setting a helpful title attribute on the icon and maybe it could link/jump to the explanation below.

greggles’s picture

Here's a screenshot with a slight tweak on the idea of Yoroy in #37:

The thumbsup/down is not a final suggestion, just indicative that there would be some kind of an icon that conveys "covered/not". There could be hover text with a quick explanation that "thumbsdown" releases will have security issues reported in the public queue and releases covered by a thumbsup will get to the private queue which means the advisory process and help/support from the security team for the maintainer as they fix the bug.

yoroy’s picture

FileSize
56.44 KB
jthorson’s picture

Thanks for the feedback.

Part of the discussion around the project applications process changes was a desire to ensure that not only was an indicator regarding SA coverage added to project pages, but that it was also made prominent. That said, I agree with the feedback provided ... the mockups I posted did go overboard, but somewhat intentionally; as the main point was to re-jumpstart the discussion. :)

Once I rebuild my drupal.org development environment, I'll look at implementing the toned-down version on an actual d.o dev site.

kattekrab’s picture

fuzzy76’s picture

I actually think the table background colors should be re-evaluated while we're at it. People might assume green means "fully supported", but it really isn't as long as it does not have security coverage.

gisle’s picture

Re #45:

There is a related issue (#2457643: Only allow releases with security coverage to be recommended) about the table background colours. The TWG recently approved a policy that only allowed stable releases (i.e. those with security coverage) to have a green background.

See the related issue for status.

drumm’s picture

Issue summary: View changes

Adding yoroy's mockup from #42 to the issue summary.

drumm’s picture

Issue summary: View changes

For implementation, following the pattern of the green checkmark on passing tests should work. See https://www.drupal.org/pift-ci-job/229847, click Show: All

This is a unicode checkmark, ✓, in a span tag for the circle background. That makes it ready for retina screens, and there are no additional images.

We’ll want to be sure this is a bit more accessible, maybe <a href="#…" title="covered by the Drupal security team">✓</a> will be enough?

gisle’s picture

Do we need the checkmark? To my eyes, the checkmark clutters the display. Its meaning is far from obvious.

It has already been suggested (see #2457643: Only allow releases with security coverage to be recommended) that only releases with security coverage have a green background in the "Recommended" section of the table. Do we need a visual indicator in addition to that?

Please see mockup below for how this would look.

Screenshot

yoroy’s picture

Thanks @drumm, the no-image approach looks good. Maybe add a font-weight: bold for the checkmark. And come up with the 2 or 3 words we can put on its title attribute.

Simpler rules for the use of background colors is very welcome too, mockup by @gisle looks good in that regard.

@gisle only using color is not enough for accessibility reasons, we can’t rely on color alone to communicate information. Of course, no icon will every be completely obvious. That’s why it would link to additional text explaining that it's about "covered by security advisories" and what that means. At the same time a simple checkmark will indicate to many something like: Yep, take this one, it’s the “best” or “recommended” version to use. Once people understand that, the checkmark is enough to help inform people’s decision which version to use.

kattekrab’s picture

@drumm - yes - great! Awesome to be able to re-use something like that.

@gisle - the simple colour background does look good, but have to agree with @yoroy on needing something other than colour here to show the difference for good accessibility practice.

Let's do this!

Thanks all.

kattekrab’s picture

Oh, we still need words for the title tags, right?

  1. Subject to security advisory policy
  2. Covered by security advisory policy
  3. Security advisory policy applies
  4. Receives security advisory notices

or...

gisle’s picture

OK. I understand the WCAG argument.

But please also use a background color different from green for versions without security coverage.

I teach a class about site building with Drupal, and my observations are that the majority of my students automatically assume that green means "secure". It my be that the checkmark fixes it, but at least in my culture, there is very common to associate the colour green with "safe", "secure", "officially approved" and so on.

The mock-up in the issue summary uses a green background for the ver. 8.x-1.0-alpha2 - which does not have security coverage.

drumm’s picture

I can think of one more design decision - currently the green/yellow coloring goes with the table headers. Releases which are supported, but not recommended, get put under the “Other” heading. See https://www.drupal.org/project/ctools for an example (8.x-3.0-alpha25 is under Other at time of writing). Should we be doing the same here?

I don’t have a personal opinion on this one. The implementation would be a bit different, instead of modifying the View, the key changes could be in the admin releases UI, making a branch without a full release un-recommendable. In the UI, we wouldn't be mixing security-supported and not; the message could go directly next to the table, and wouldn't be necessary to repeat every row. It would remove the option to have maintainer-recommended releases which are not security-supported.

This also can affect <recommended_major>1</recommended_major> in the update status XML. However, I think the presence of that doesn't make a visible difference in the core update status UI, so it probably doesn't matter too much.

gisle’s picture

Issue summary: View changes
FileSize
19.94 KB

I can think of one more design decision - currently the green/yellow coloring goes with the table headers. Releases which are supported, but not recommended, get put under the “Other” heading. See https://www.drupal.org/project/ctools for an example (8.x-3.0-alpha25 is under Other at time of writing). Should we be doing the same here?

In comment #8 of #2457643: Only allow releases with security coverage to be recommended, jthorson (from the TWG) says that maintainers could have "non-stable" recommended releases (i.e. they should appear in the "recommended" section of the table, but with a yellow background).

His argument is:

This would be a much easier and less intrusive change than not allowing 'non-stable' recommended releases, which would require a mass update of existing non-stable 'recommended' releases which don't meet the new criteria. For this reason, I would suggest limiting this to simply updating the display color of those releases; which I think addresses the main spirit of the proposal.

I amended the proposal accordingly and this was also the design that finally was approved by the TWG.

(However, I don't care whether you put it under "Recommended" or under "Other", as long as it is not green.)

Mockup in issue changed to reflect the design suggested by jthorson and approved by the TWG.

fuzzy76’s picture

Allowing maintainers to recommend releases without security coverage is a terrible idea to begin with IMHO. It sounds like a separate issue for one of the workgroups, but I'm a bit unsure which one.

gisle’s picture

fuzzy76 wrote:

Allowing maintainers to recommend releases without security coverage is a terrible idea to begin with IMHO. It sounds like a separate issue for one of the workgroups, but I'm a bit unsure which one.

It sound like something the TWG should decide. Quoting from its project page:

The TWG's job is to ensure that the necessary policies, procedures, and standards exist to address technical concerns affecting the Drupal community. This includes items that affect all projects, such as the Drupal coding standards, Git repository usage and licensing policies, and Drupal's full-project approval process.

Except that the TWG has already made a decisision.

Recapping the history of this related issue #2457643: Only allow releases with security coverage to be recommended:

  1. It started out by me proposing that only releases with security coverage could be tagged as "recommended".
  2. In #8 jthorson of the TWG suggested that the change should only affect background colour, not "recommended" status.
  3. In #9 I (as the OP) says "I'm fine with .. #8". Nobody else commented
  4. In #10 jthorson says that the proposal (as described in the changed issue summary) has been "TWG approved as a suggested recommendation".
  5. In #19, it was closed by drumm with instructions to merge into this issue.

If you guys want to go another round with the TWG (or whatever working group this belongs under) I am fine with that as well, I just wanted to point out that this has already been before the TWG and a decision has been made.

catch’s picture

Yes that would need a new TWG decision. For example you might have a 1.x branch with a beta, that's never going to get a stable release. Then a newer branch that's also on beta, but is headed towards rc. In that case you want to push people towards the newer branch, even though neither are released yet. Same as if you have a 1.x and 2.x branch, the 1.x branch has a stable release but won't get further development, the 2.x branch has an RC - you want to steer new users towards the RC of the newer branch so they don't need to upgrade later. So security support and recommended branches are really two different things.

jthorson’s picture

I think that Catch's description in #58 describes our current status quo. A proposed policy that maintainers not be allowed to recommend non-stable releases would have to be put forward as a new TWG issue, though my initial suspicion is that it would not be successful for the reasons identified above.

drumm’s picture

Ok, works for me.

This will be altering the project_release_download_table View. Drupal.org-specific information about the security process will make its way into the table, which we don’t want in Project module. Rather than keeping track of altering it via hooks, let’s clone the View and manage it in the drupalorg_projects Feature.

(The project_downloads View was made for the /project/drupal page changes at the time of the 8.0.0 release. It may or may not be usable for this issue.)

jthorson’s picture

I originally took a look at simply adding this as a new column/field on the project_release_download_table view (instead of adjusting the existing field); by adding the version-extra field to the view, and then adding a custom display formatter that outputs the checkmark for 'null' and no image for any other value.

This approach ran into a snag, in that adding display of the version-extra field would silently fail via the Views UI.

Drumm ... I wanted to bounce this off you as an alternative solution, as this would make it a Views only change, with no custom code. Thoughts?

drumm’s picture

Yes, we should use Views, and export the whole View without custom altering code. (Assuming that’s possible.) We do need to clone project_release_download_table since the View configuration will be Drupal.org-specific.

I’m not sure why adding that was failing in Views UI. Investigating that is the next step here.

jthorson’s picture

The parameter is already in the view as a sort parameter, it was just adding it as a display field which was failing.

Does Views support a conditional output rewrite, where we can add the checkmark to the Content: Version field, based on whether or not Content: Version-extra has a value? Unless it does, I'm not sure how we implement the mockup in the issue summary without any custom code.

If we can put the checkmark in it's own column, and sort out why adding version:extra as a display field is silently bailing, then we can do this using the 'no results' and 'rewrite output' options, and no custom code should be needed. (Except, perhaps, new code for a display handler associated with that field, which I suspect may be missing in versioncontrol_git.)

Edit: added "version:extra" for clarity

drumm’s picture

If we can put the checkmark in it's own column, and sort out why adding version:extra as a display field is silently bailing, then we can do this using the 'no results' and 'rewrite output' options, and no custom code should be needed.

Yes, and the new field can be grouped into the Version column using the Table format options.

The background will probably need a class for the row, which will need a bit of custom code. Project Issue does this in project_issue_preprocess_views_view_table().

jthorson’s picture

Yes, and the new field can be grouped into the Version column using the Table format options.

Ah, perfect. That was the missing link for me. :)

jthorson’s picture

Seeking feedback on the implementation currently live at jthorson-drupal.dev.devdrupal.org.

... Other than the 'no security releases for D6 versions' comments ... realized that, and am working on an alternate approach which would allow us to distinguish between them. :)

yoroy’s picture

Can't visit project pages, e.g. https://ci-drupal.redesign.devdrupal.org/project/views gives me 403 Forbidden.

greggles’s picture

@yoroy try this - https://jthorson-drupal.dev.devdrupal.org/project/views I think you were on the wrong subdomain.

jthorson’s picture

Proposed implementation now live on my dev site.

Examples:

Project with D8 release: https://jthorson-drupal.dev.devdrupal.org/project/admin_toolbar
Project with D6/D7 releases (showing D7 covered, but D6 not): https://jthorson-drupal.dev.devdrupal.org/project/views
Project without releases: https://jthorson-drupal.dev.devdrupal.org/project/edit

Seeking reviews ...

drumm’s picture

A few small implementation changes I think would be good:

  • Use .security-coverage-checkmark as the CSS selector instead of span.security-coverage-checkmark. Not matching the element name is slightly faster, and makes the CSS a bit easier to work with.
  • The link that should be to the security info is https://jthorson-drupal.dev.devdrupal.org/project/title= instead of something like #drupalorg-security-info
  • Can we shorten the message to:
    <div id="drupalorg-security-info"><span class="security-coverage-checkmark">✓</span> Stable releases in supported branches receive <a href="https://www.drupal.org/security-advisory-policy">security advisory coverage</a>. <a href="https://www.drupal.org/node/467020">Read more about stable versus non-stable releases</a></div>
    (This also removes the messages class, which isn’t well-styled on its own.)
jthorson’s picture

I found that the text felt a bit awkward when sitting alone under the download table ... the border that came with the messages class helped address that. In removing the messages class, are you suggesting removal of the box as well; or keeping the box as is and simply using dedicated styling instead of piggybacking on the existing element styles?

yoroy’s picture

FileSize
87.48 KB
166.43 KB

Thank you @greggles, I somehow ended up there while following @jthorson's link.

The current solution, it's still… aesthetically challenged :-)

I assume we really want to avoid loading images here, else we could reuse existing checkmark icons and style the message underneath accordingly:

As you can see I moved the checkmark icon next to the download links and tried to make it a bit smaller still:

I think it's fine to just wrap the text beneath in a simple p-tag. I tried to cut some words as well.

Following Drupal's Security Advisory Policy, only stable project releases in supported major version branches get security advisory coverage. More about stable versus non-stable project releases.

Does this still get the point across?

Styling for the icon in that last mockup:

.security-coverage-checkmark {
    background: #367d02;
    border-radius: 50%;
    color: #fff;
    display: inline-block;
    font-size: 11px;
    height: 13px;
    line-height: 13px;
    min-width: 13px;
    text-align: center;
}
yoroy’s picture

Or even:

Only stable project releases in supported major version branches get [security advisory coverage]. More about [stable versus non-stable project releases].

<p><span class="security-coverage-checkmark">✓</span> Only stable project releases in supported major version branches get <a href="https://www.drupal.org/security-advisory-policy">security advisory coverage</a>. More about <a href="https://www.drupal.org/node/467020">stable versus non-stable project releases</a>.</p>
greggles’s picture

The text in 73 seems accurate/complete to me and is a nice short length.

I wonder if "green checkmark" will imply more than we want. It seems ok to me to use a new image if someone can think of a different icon that focuses on the point a bit more.

jthorson’s picture

The current solution, it's still… aesthetically challenged :-)

I agree ... thus the request for feedback. I guess I could have been clearer in that this was a 'proposed implementation approach', not 'proposed implementation'. :) Thanks for the suggested improvements.

I wonder if "green checkmark" will imply more than we want.

Perhaps something like an 'SA' inside a circle, since we're really talking about Security Advisor coverage?

Resized checkmark and updated wording are live on the dev site (but not committed to branches yet). Checkmark is still in the first column, as I didn't immediately see where I could append it to the rewritten extension field (and didn't have time to dig into it today).

drumm’s picture

I assume we really want to avoid loading images here

Another image would be okay to add if needed. The ✓ is convenient since it is text and resolution-independent already, but of course not useful if it doesn’t communicate well. If we go with an image, we should use an SVG. (Or a PNG sized 2x if it isn't a vector image.)

jthorson’s picture

Speaking with drumm and mlhess at NOLA, we decided to try a shield image instead of checkmark icon. The dev site has been updated, but needs someone to build us a shield image ... the current implementation is 12x12px.

yoroy’s picture

yoroy’s picture

kattekrab’s picture

Issue summary needs an update to show the shield, in place, etc... as per the now "fixed" #2721227: Design a security shield icon shield issue.

kattekrab’s picture

hestenet’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update
FileSize
40.09 KB

Here is the sheild icon from #2721227: Design a security shield icon Shield Icon

And here is one mockup of how it might be used:

Shield icon on download table mockup

hestenet’s picture

Issue summary: View changes
hestenet’s picture

Issue summary: View changes
yoroy’s picture

Status: Needs review » Needs work

Does it really have to be that big and put in front of the other information? This way it looks like we're yelling again. We're presenting it as a big selling point here, but security is a hygiene factor, not a selling point.

The design of the icon was optimised for small dimensions, it doesn't really work on that larger scale. Doesn't really help to distort the aspect ratio either.

#72 is a complete proposal, what were the considerations for this different approach?

gisle’s picture

I still think is counter-intuitive that the the release "8.x-1.0-alpha2" in the latest mockup (#82) is shown with a reassuring green background. If I've understand the arrangement correctly, this is an alpha-release so does not receive security coverage. Still, the green background colour communicates to me that this release is "safe".

Personally, I am just confused by the "badge" pictogram that now has been added to the design. I understand that some sort of pictogram is needed to also conform to WAI-standards, but why not a standard checkmark as shown in #72?

kiamlaluno’s picture

I much prefer what shown in #72, even if I would put the checkmark on first row, before the version number. I don't see anything confusing in the row background, as that is used for another meaning. If the green checkmark is going to be used to say which versions get security advisories, the message seems clear, to me: The rows without the green checkmark don't get any security advisory.

catch’s picture

Agreed with #85. The massive shield looks like the whole project is being promoted as secure, I think just the smaller icon in the release table would be plenty.

Although for me pesonally I'd rather invert this, and have security-supported releases in green, then non-security-supported releases in yellow. We're promising less with the policy change, not more - right now this looks like a much bigger promise.

gisle’s picture

kiamlaluno wrote:

I don't see anything confusing in the row background, as that is used for another meaning. If the green checkmark is going to be used to say which versions get security advisories, the message seems clear, to me: The rows without the green checkmark don't get any security advisory.

I did actually user test this at one point, asking users (the users were site builders that were familiar with WCMSes in general, but none of them knew about Drupal) to indicate what releases they consider "secure" by presenting them with mockups where there were "checkmarks" on some releases and/or a green background on others. The meaning of the "checkmark" was explained in a text placed below the table, like in #72, and no explanation of the meaning of having a green background. Almost all these users picked the releases with the green background, including one without the checkmark. If there was a checkmark on a yellow backround, it was ignored. I was not surprised, as in my experience, users don't read advisory text - they scan for visual clues.

And while this test was based on the original "checkmark" design, I don't think having a "badge" placed next to the release will change this finding. To me the "badge" just suggests that this particular release has won an award or something.

As for the "another meaning" of the row background hinted by kiamlaluno, I don't think this meaning is documented anywhere near the project page. How many users know the meaning of the green background? And do we really want maintainers to be able to "recommend" a release with no security coverage?

PS: At one point, I actually got the TWG to agree that we only should allow releases with security coverage should have a green background (see this issue: #2457643: Only allow releases with security coverage to be recommended). And then it of course it was closed and forgotten.

But let me humbly suggest that before this is implemented, you at least try some user testing with mockups to determine that the following assumptions underlying this design are true:

  1. That users understand that the "badge" indicate "security coverage".
  2. That users don't think that the background colour green indicate that the release is "secure".

And by users, I mean real users trying to discover what Drupal module they should pick - not veterans that has been around Drupal since the previous century and know all the secrets.

kiamlaluno’s picture

It all depends from how the test was done. Did the projects used for the test all have a single stable release?
I find difficult to believe that users could confuse the meaning of a symbol with the background color. If I were shown a table with some of the rows showing an asterisk in the same position, and an explanation of what the asterisk means, I would not get the asterisk is for all the rows with the same background color. That in every site, even in a site I see for the first time.

kiamlaluno’s picture

Also, to be sure every user understands the same thing, symbols should be avoided. This is probably going to be less confusing, if we replace the thumbs-up with yes, and the thumbs-down with nothing.

screenshot

gisle’s picture

It all depends from how the test was done. Did the projects used for the test all have a single stable release?

No, there was several stable releases to choose from. One of them was "recommended", but had not received security coverage.

This was the mockup used for test:

User test

You're apparently one of the few people that actually read explanations. Most people don't.

kiamlaluno’s picture

If we are going to do something for users who don't read, we are good. I cannot imagine what they understand from a shield, without reading. :D I just hope they don't play Despicable me - Minions Rush, or the shield will remind them of the game.

Actually, it seems I am one of few people who don't confuse a symbol with the background used in a table.

kiamlaluno’s picture

That is probably why the test didn't work. The test should use more than one image, possibly using a table that has a header also for the image, or the word used to mean covered by security advisories.

gisle’s picture

Just to make my intentions clear.

I am not suggesting replacing the pictogram with the background.
Nor do I suggest that we change the meaning of the green table background to mean something other than "recommended release".

What I suggest is this: Do not permit a maintainer to tag a release as "recommended" as long as that release does not receive security coverage.

The intentions behind this proposal is to create a small dis-incentive for those maintainers that keep their projects in perpetual beta to avoid having them vetted by the security team (e.g. Feeds, Field collection).

kiamlaluno’s picture

IMO, it should be the users who understand that if they use versions not covered by security advisories, they do it at their own risk. The problem is when that becomes the normality, and I am not sure it can be done anything about that. I don't think we can push developers to create a new stable release every X months.

My notes are about how to implement the feature, not the motivations.

  • Do the users associate the symbol color with the color of the table row background? Don't use colors for the symbol.
  • Don't they notice the symbol because it seems put in a random position? Add a column for the symbol, with its own header.
  • Is there the risk users misunderstand the symbol? Add new column (e.g. Security coverage or Security advisories) for yes/no (to use instead of the symbol), with its own header.
hestenet’s picture

@kiamlaluno - That's good feedback - I think a separate column for the symbol is important, and I think you're right about the colors being confusing.

There are some mockups of how this could look on the project page as a whole here: #2035277: Draft text for security advisory coverage messages

mrf’s picture

re: "The intentions behind this proposal is to create a small dis-incentive for those maintainers that keep their projects in perpetual beta to avoid having them vetted by the security team"

The security team does not do any active vetting of projects. The security team waits for users of the module to report issues. The only difference between a security issue found in a 1.0 module vs a beta and sandbox is whether or not the resolution of that issue will have an accompanying security advisory published on Drupal.org

Maintainers are keeping their projects in perpetual beta to accurately reflect the number of bugs a user might expect using that module. I really don't think SA coverage factors into their decision very often.

If the security team policy I describe above is still is an accurate description of what the security team is doing whatever type of visual language is used here should be subtle enough to reflect the nuance that "someone with a security background didn't necessarily review this code but if they had and found something it would have a public advisory".

drumm’s picture

On backgrounds: there’s been some at the Drupal Association planning to remove the green/yellow/red backgrounds like https://www.drupal.org/project/drupal. That was snuck in for the D8 launch, I haven't heard much feedback on that specifically, positive or negative. Rushing that to all of contrib wouldn't have been a good idea at all. I'm tempted to say we might want to do that now, but I don't really want another bike shed.

drumm’s picture

Assigned: Unassigned » drumm
FileSize
60.93 KB

Cross-posting the implmentation from #2035277: Draft text for security advisory coverage messages

Screenshot

I cleaned up the “Project Information” section a little, removing the odd margin and “Last modified” styling. I could see more icons coming into this section and everything getting a left margin again, but that's another issue.

I think that’s the best place since that area is (mostly) trying to summarize project quality. Especially with that being the one icon, I think it gets enough emphasis.

  • drumm committed 2b620c8 on 786702-security-coverage
    Issue #786702 by mr.baileys, greggles, yoroy, jthorson, drumm, hestenet...

  • drumm committed 6501d3a on 786702-security-coverage authored by yoroy
    Issue #786702 by yoroy: Add the shield icon
    
kiamlaluno’s picture

I like #100. I still think it should be clear there is an icon for every row on the releases table, and not an icon for every group (whatever users would think as group).

drumm’s picture

I forgot to mention - I haven’t gotten to the downloads themselves, those are coming up soon.

  • drumm committed aa24c30 on 786702-security-coverage
    Issue #786702: Update for configurable downloads
    
  • drumm committed ea3da9c on 786702-security-coverage
    Issue #786702 by jthorson, drumm: Add information about releases covered...
drumm’s picture

Issue summary: View changes

Adding deployment notes draft.

drumm’s picture

Screenshot

I finally got the shields for each release working well.

The final todo is to move the non-full releases to the Other group.

drumm’s picture

Status: Needs work » Needs review

Merging in #2457643: Only allow releases with security coverage to be recommended has proven to be more difficult than expected.

The download table is 4 displays of a View. I added the must be full release restriction to the “Recommend releases” display, that went well. To get them added to the “Other releases” table, I added an OR group to the filters. The query is coming out with JOIN condition

taxonomy_term_data_node__field_data_field_release_recommended2.field_release_recommended_value != '1'

and WHERE condition

taxonomy_term_data_node__field_data_field_release_recommended2.field_release_recommended_value = '1'

… which of course matches nothing. Debugging that is looking like a tour through Views internals or redoing how the View is built in general to avoid this.

In the interest of getting the work done here moving along, I’ll reopen #2457643: Only allow releases with security coverage to be recommended with more details.

  • drumm committed 2b620c8 on 7.x-3.x
    Issue #786702 by mr.baileys, greggles, yoroy, jthorson, drumm, hestenet...
  • drumm committed 6501d3a on 7.x-3.x authored by yoroy
    Issue #786702 by yoroy: Add the shield icon
    
  • drumm committed aa24c30 on 7.x-3.x
    Issue #786702: Update for configurable downloads
    
  • drumm committed ea3da9c on 7.x-3.x
    Issue #786702 by jthorson, drumm: Add information about releases covered...
drumm’s picture

Status: Needs review » Fixed
Issue tags: +needs drupal.org deployment
drumm’s picture

This is on staging now: https://www.staging.devdrupal.org/project/ctools

I plan to deploy about an hour from now.

mrf’s picture

Status: Fixed » Reviewed & tested by the community

Stage looks good to me.

mrf’s picture

Status: Reviewed & tested by the community » Fixed
drumm’s picture

This has been deployed.

kiamlaluno’s picture

It seems there is something wrong with https://www.drupal.org/project/views_slideshow: Only the 8.x-4.0 version gets the shield, but not so the 7.x-3.1 version. The maintainer marked as recommended both the versions; the only difference I see is that the 8.x-4.x branch is the default one, but that should not make any difference.

screenshot

kiamlaluno’s picture

Status: Fixed » Active
FileSize
16.32 KB

Also, it should not appear on Drupal 6 versions, as in https://www.drupal.org/project/ctools. Those surely don't get any coverage from the Drupal Security Team, since Drupal 6 is not supported anymore.

mrf’s picture

I'm also seeing issues with https://www.drupal.org/project/backup_migrate (7.x-2.8 missing shield) and https://www.drupal.org/project/entityreference (7.x-1.1 missing shield)

drumm’s picture

Looks like those were all created in 2013. I'm guessing those nodes were saved/migrated slightly differently. I'll be able to investigate later tonight or tomorrow AM.

drumm’s picture

This affects 3,304 releases made before 2014-01-14, and 6 made on 2015-11-12, if those are still the current ones.

They have an empty string in the field_data_field_release_version_extra table, more recent releases don't have a row in that table. Saving the an affected release node makes it match the newer ones.

I’ll delete those rows and clear the appropriate caches.

drumm’s picture

views_slideshow, backup_migrate, and entityreference are now fixed up.

drumm’s picture

What I’d really like to do for D6 projects is remove the ability for them to be in the download table. We did this briefly very soon after the D8 launch, but we heard loudly from maintainers that really wanted to actively support their D6 projects. We should do this, but it either has to be communicated/coordinated well or done so late that no one notices.

That said, #2532062: [policy, no patch] Changes to the security team process / tools to reduce workload from increased project applications will mean projects can opt out of security team coverage, so this can’t be the simple show-if-full-release Views field we have now. I can look into the best way to filter for D6 tomorrow.

  • drumm committed 17ff4b1 on 7.x-3.x
    Issue #786702: Use a new handler instead of field hacks
    
drumm’s picture

Status: Active » Fixed
Issue tags: +needs drupal.org deployment

The fix is pushed to https://www.staging.devdrupal.org/project/ctools.

Since there’s going to be more of a need to do more logic around what has security coverage or not, the custom Views handler should serve us well. It is a lot more readable than tripling down on Views hacks that would have served us well if coverage remained straightforward. And it avoided the apparent Views UI bugs that have plagued this issue.

drumm’s picture

This has been deployed.

dgtlmoon’s picture

Sorry to be a huge pain

Alt text is meant to be an alternative information source for those people who have chosen to disable images in their browsers and those user agents that are simply unable to “see” the images.

Image title (and the element name speaks for itself) should provide additional information and follow the rules of a regular title: it should be relevant, short, catchy, and concise (A title “offers advisory information about the element for which it is set.“). In FireFox and Opera it pops up when you hover over an image:

<img src="/sites/all/modules/drupalorg/drupalorg/images/shield-icon.svg" alt="Full release covered by the Drupal Security Team">

I really feel you need an ALT and TITLE here, I'm waving my mouse over the icon without any obvious information at hand, yeah it's in the project body, but I feel a TITLE would help also

drumm’s picture

I committed the same text as alt to be title too. This is in the dev branch and will be deployed later this week.

The fact that it is a shield doesn't really add useful information if you can't see the symbol, so I think the alt text is okay. I think screen readers usually don’t read title attributes, so the duplication shouldn't be a problem. Please correct me if I'm mistaken.

drumm’s picture

(restoring issue credit, I unchecked the boxes for the commit message)

dgtlmoon’s picture

@drumm so it's not just about screen readers, it's also about us fortunate people with perfect sight who like to mouse over things to find out what they're about, in this case title= would be fantastic and is no way duplicating

  • drumm committed a57e001 on 7.x-3.x
    Issue #786702: Add title attribute to shield image
    
drumm’s picture

The title attribute has been deployed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.