In issue.inc v1.233, the test for allowed extensions for attached file seems to be incorrect: issue.inc/project_issue_validate_file apparently exits with a form_error if an extension is not allowed for each role of the user instead of failing it is is not allowed for any of them as it seems that it should.

A similar test for extension permission is performed in project_issue.module/project_issue_munge_filename v1.28, but with a different logic, which seems to be correct: permissions for each role of the user are accumulated to the whitelist before testing, instead of testing/rejecting on each role separately.

Since it looks like this test could/should be refactored so that only one code and logic is used, I'm not submitting a patch unless you greenlight my so doing, because this would probably add another function and possibly affect other parts of the module.

CommentFileSizeAuthor
#15 project_issue_extension_whitelist.patch_5.txt7.39 KBdww
#12 project_issue_extension_whitelist.patch_4.txt7.4 KBdww
#10 project_issue_extension_whitelist.patch_3.txt7.35 KBdww
#7 project_issue_extension_whitelist.patch_2.txt3.84 KBdww
#3 project_issue_extension_whitelist.patch.txt3.84 KBdww

Comments

dww’s picture

dww
we/he/they
commented

Yes, it'd be great to see this fixed before the 5.x-1.0 release (which will be out any day now, probably). Sorry I overlooked this issue earlier...

fgm’s picture

fgm
Primary language French
Location Paris, France
commented

Could you confirm

  • in which file you want the refactored function to do (project or project_issue ?) I'd say project because it can exist without project_issue, but the opposite is not true
  • on which version of the file I should base the patch
dww’s picture

dww
we/he/they
commented
Assigned: Unassigned » dww
Status: Active » Needs review
StatusFileSize
new project_issue_extension_whitelist.patch.txt3.84 KB

These settings are only for issue attachments, so that's where things should go (for now). Please review and test the attached patch, it works under my limited testing, but it'd be great to get more eyes on this.

dww’s picture

dww
we/he/they
commented

Oh, and as per the version listed here, this patch is for HEAD. We can backport it once we're happy with the code.

fgm’s picture

fgm
Primary language French
Location Paris, France
commented

Looks good to me.

Just two minor things:

  • spelling: use "extension", not "extention"
  • coder.module recommends using true/false/null instead of TRUE/FALSE/NULL for performance reasons, so maybe you'd want to switch to this format. Some might argue that drupal should use uppercase, based on http://drupal.org/node/5993, but these are not user constants, but actual language builtins, and the uppercase form just happens to be transformed to lowercase.
fgm’s picture

fgm
Primary language French
Location Paris, France
commented
Status: Needs review » Needs work

Forgot to add that PEAR, upon which Drupal rules are based, use the lowercase form too, just like Zend Framework.

dww’s picture

dww
we/he/they
commented
Status: Needs work » Needs review
StatusFileSize
new project_issue_extension_whitelist.patch_2.txt3.84 KB

- new patch fixes the spelling typo, thanks. ;)

- TRUE vs. true is a much bigger debate. so long as core continues to use TRUE (and the vast bulk of drupal code in contrib), I'm sticking with that for consistency, even if there's a negligible speed improvement to using true...

fgm’s picture

fgm
Primary language French
Location Paris, France
commented
Status: Needs review » Reviewed & tested by the community

Another minor tweak might be useful, and that is adding the "odg" extension to the whitelist. As one of the OpenDocument variants, it fits right there with the three others, not to mention the fact that users submitting issues are probably more likely to submit a drawing (odg) than a presentation (odp).

I know this can be changed after the fact, of course, but I think it would be best to include it in the default list.

dww’s picture

dww
we/he/they
commented
Status: Reviewed & tested by the community » Needs work

Upon closer inspection, this is screwed up in a few ways. I'm re-rolling a new patch now. Stay tuned for details once it's ready...

dww’s picture

dww
we/he/they
commented
Status: Needs work » Needs review
StatusFileSize
new project_issue_extension_whitelist.patch_3.txt7.35 KB

New patch:

  • Adds a UI to define the default extensions if there are no per-role extensions defined.
  • If any of the users's roles define an explicit list, only those are used (instead of always using the default list for all roles).
  • Only roles that currently have the 'create project issues' permission check the corresponding variable, which protects us from the case where a role had some extensions defined, but then was changed to no longer provide the permission.
  • If 'authenticated user' has the perm, the admin UI (and checking logic) assume that all custom roles also provide the permission, and provide fieldsets for those roles, too. This is accomplished by means of the new private helper method, _project_issue_roles() -- see the phpdoc, and code comments.
  • Adds odg to the list as per fgm's request
hunmonk’s picture

hunmonk commented
Status: Needs review » Needs work
  • there's an extra space in the description for 'project_issues_extensions_default'
  • isn't it overkill to use a collapsed fieldset for one textfield?
  • need to use the drupal constants for anon and auth users instead of the integers in _project_issue_roles
dww’s picture

dww
we/he/they
commented
Status: Needs work » Needs review
StatusFileSize
new project_issue_extension_whitelist.patch_4.txt7.4 KB

- extra space in description removed
- constants used (those are great, makes the code much more readable -- i didn't realize they existed). ;)
- re: fieldsets -- it matches how core handles this at admin/settings/uploads -- if it's good enough for core, it's good enough for me. ;)

thanks,
-derek

hunmonk’s picture

hunmonk commented
Status: Needs review » Needs work

i think i'd prefer to lose the $sort variable in _project_issue_roles -- what are we usually sorting there, maybe 6 or so role names? i'd rather give up the tiny performance hit there in order to keep that code simple.

hunmonk’s picture

hunmonk commented

...or move the sorting to the settings page form builder...

dww’s picture

dww
we/he/they
commented
Status: Needs work » Needs review
StatusFileSize
new project_issue_extension_whitelist.patch_5.txt7.39 KB

Sure, there's only one place that needs it sorted, that form builder, so we can do the sort there. I was actually thinking of this as I was working on #10, but I decided it would be handy to just make it an arg. However, I'm fine with the simpler version for now, and if it turns out we need to use this more often and want it sorted more often, we can always change it in the future.

dww’s picture

dww
we/he/they
commented

Forgot to mention, my latest patch also:

- adds a constant for the list of default extensions
- moves the defaults outside of a fieldset (to the top of the form) and changes the wording of the title and description

hunmonk’s picture

hunmonk commented
Status: Needs review » Reviewed & tested by the community

code style is good. properly falls back to defaults if no roles contain allowed ext. properly sums exts across roles. properly disallows files not on the whitelist. properly displays all roles if auth role has create perm.

dww’s picture

dww
we/he/they
commented
Status: Reviewed & tested by the community » Fixed

Committed to HEAD, DRUPAL-4-7--2 and DRUPAL-4-7. Thanks.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)