add extra validation to release node form for security update releases

sorry, i didn't mean the entire SA... just the SA identifier, e.g. "DRUPAL-SA-2008-008", etc.

upon further thought, the SA identifier might not help much, unless we make it more complicated like adding a new table of identifiers mapped to project nids, and the sec team can create/allocate new ids, etc. if it's just a text field with little or no external validation, people will just input jibberish in there. this might be getting too complicated. perhaps the extra description and confirm form are enough.

see also http://drupal.org/node/212219

I agree w/ greggles. Once it's published and out, it's too late to go back and call that a security release with all the various implications that has for end users. We should just remove it from the choices, and perhaps add some fine print help text saying something like "If you want to mark this release as a security update, contact the security team." I don't think the sec team has a process for this case, but preventing it is a good first step. ;)

Looks like seanr isn't actually working on this...

Seemed like no one's ever going to work on this unless I do it, so I just took an initial stab at this.

A) When adding a new release, there's now a (perhaps too verbose) description.

B) When editing an existing release that's not marked as a security update, the "Security update" option is gone for users without the 'administer projects' permission.

C) When editing an existing security update release, the entire 'Release type' selector is disabled for users without the 'administer projects' permission and there's a description.

See attached patch and screenshots.

This patch is also running on http://project.drupal.org, so you can try to see it there if you already maintain projects on d.o (though it's going to be hard for you to add new releases since you can't tag the project.d.o CVS repository).

Here's a first draft of adding some kind of "Are you sure?!" validation when people select 'Security update' on a new release.

I was imagining a dynamic checkbox called "Are you sure you want to mark this as a 'Security update'?" which is hidden by JS until the user selects the 'Security update' term. If that term is selected and the checkbox isn't checked, it'd be a validation error for the form. That seems a lot easier than trying to add an entire new page on the node form which acts as a confirm form or something.

However...

If we put a checkbox inside the "Categories" fieldset, core's (rather lame) hard-coded handling in taxonomy_nodeapi() will get confused unless we jump through a lot of hoops to try to undo the checkbox before taxonomy_nodeapi() is called.

If we put a checkbox outside the "Categories" fieldset, I'm worried it's going to look wonky just floating around outside a fieldset. But, security updates are rare, and I guess it doesn't matter *that* much how aesthetically pleasing the form looks in that case. By moving the "Release type" vocab to the bottom of the "Categories" fieldset, this isn't that much of a problem...

So, this is *almost* all working, except I've never tried to use jQuery to attach to a multi-select form element, so that part isn't working. The JS part of this patch is broken, and the confirm checkbox is always visible, even if you didn't select "Security update", and I've commented out the lines that automatically hide the element by default and load the .js file, etc. Anyone with jQuery fu who wants to help with this would be most appreciated.

Also, I don't remember if FAPI ever marks checkboxes in the red error box when there's a form error. It's not happening with this patch, and I'm not sure if that's something I'm doing wrong or a limitation in FAPI itself.

Thanks for the review.

- Right, only #13 is applied on p.d.o -- until the JS is working I didn't want to deploy #14.

- Agreed that http://drupal.org/security-team#report-issue would be better.

- Seems like there's an edge-case bug in the logic somewhere -- it shouldn't hide the 'Security update' term if that's already selected.

- #15.1: Hrm, yeah, good point. I'm worried about the fact that a lot of people have 'administer nodes' on d.o, and any time I relax project* related validation for those folks, we get problems because the admins break something with their extra powers. So, I think I'd prefer (in drupalorg.module) to remove the "Published" checkbox from unpublished release nodes unless you have 'administer projects'. That should happen regardless of the security update, too, so I'm splitting that off into #346618: Remove "Published" checkbox from unpublished release nodes for most users. To avoid patch conflicts, let's just handle that (easy part) once the rest of this lands.

- #15.2: I agree we need that, but I was thinking that's out of scope for this issue. It seems like something the packaging script should do, although I guess it could be done via a custom submit handler for the release node form. Either way, I think that should also be a separate issue. This patch is already getting huge, and I don't want to make it any harder to review/test/commit. So, let's talk over at #346620: Notify security team whenever releases are marked "Security update".

- #15.3: Good points, thanks for raising them, but yeah, I don't know what else we can do about it right now.

Anyway, these patches clearly need work for the bug aclight described, the better "contact" link, #14 needs jQuery multi-select fu, and neither one addresses #212219: Dev package showing as security update yet.

Might be overkill, but once a release is marked 'Security update', it might be nice if there was an auto-generated note when viewing the unpublished node that was something like:

"Since this release is a security update, it will not be published until the Drupal Security Team has published the security advisory and manually publishes this release. If you think this has been done in error, please contact the security team."

p.s. It'd also be nice to either update that "Types of releases" handbook page to more explicitly describe what's up with security updates and update_status, or add a new sub page specifically about that which can be linked from all these various places...

- Adds PHP constants for the various URLs we're using so we can change them in 1 place.
- Uses http://drupal.org/security-team#report-issue for contacting the sec team.
- Removes the 'Security update' option for all -dev release nodes, even for project admins.

Also, I just deployed this on p.d.o and ran the DB update to set the system weight for drupalorg heavier than cvslog. I wonder if that was the problem aclight was seeing on p.d.o.

TODO:
- see if aclight's reported bug from the top of #15 is still happening on p.d.o now that the weight is right.
- jQuery multi-select fu
- #17 if we want that

Got some jQuery multi-select help from dmitrig01 and merlinofchaos in #drupal IRC just now, and that's now working. Yay! This current patch (#19) is now deployed on http://project.drupal.org if you want to try it there.

Setting this back to CNR, since it'd be nice to get confirmation if the system weight thing solved aclight's problem, and to get more feedback on all this. I think #17 can be punted to another issue if we want it.

There was a bug when you select "Security update" and preview where the checkbox disappeared on the new page load. That's because I was always adding the "js-hide" class, forgetting about the preview case. For some ungodly reason, $form['taxonomy'][$vid]['#default_value'] is an empty array even when some elements are selected on preview, so I'm inspecting $form['#post'] directly. Ugh. I don't know if this is something going wrong in all this nasty FAPI altering code, or if that's expected behavior. At least it works. ;)

I think I've found a jQuery bug, however. Whatever values are selected when you hit preview, the jQuery always thinks those are selected on the new page, in addition to whatever else you've actually selected. :( So, it means that if you selected 'Security update' on the first page, the checkbox never disappears on the preview page, even if you deselect that option. Not sure what to do about this, but I suspect it'll go away when we upgrade to D6 and jQuery 1.2, so I don't want to spend much more time on it.

After more UI discussion in IRC, we decided to move most of the warning text about what happens when you select 'Security update' as a description on the confirm checkbox, instead of always displaying it under the "Release type" element. Furthermore, we simplified the text to just be What is a release type?.

This is now deployed on p.d.o. See a new round of attached screenshots, if you prefer.

I think this is about RTBC now...

chx: confirmed there's no more elegant way to check for which terms are currently selected in "Release type" to decide what classes to put on the div in the prefix for the "Are you sure...?" checkbox. If they were both the same form element, we could just use #process, but since they're different form elements, that won't work. Since we neither store nor print the value from #post, there's no security issue here.

Therefore, committed this to HEAD and deployed for real on d.o and p.d.o.

@dgtlmoon: No, since workflow isn't running on d.o and we're not going to add new dependencies for something like this.