ISLANDORA-2096 fix download menu permssion.

[mjordan]

JIRA Ticket: (link)

https://github.com/Islandora/islandora_premis/pull/70

What does this Pull Request do?

Enforces the download premis metadata permission.

What's new?

Added an if (islandora_object_access('download premis metadata', $object)) check to the delivery callback for the PREMIS XML file, and appropriate HTTP status code to cover cases where user does not have this permission.

How should this be tested?

In the 7.x branch,

1. As a user who does not have the 'download premis metadata' permission, visit the URL for downloading a PREMIS XML file, e.g. http://localhost:8000/islandora/object/test:1/manage/premis/download. The file will download.
2. Check out the 7.x-ISLANDORA-2096 branch and clear your menu cache, for good measure.
3. As the same user, try to download the PREMIS file. You should get a 403 Unauthorized response.

Interested parties

@adam-vessey @rosiel @DiegoPino

[mjordan]

@adam-vessey I've got an alternative implementation that incorporates your suggestion at https://github.com/Islandora/islandora_premis/pull/70#issuecomment-336559687. If you think that one is better than the one in this PR, let me know.

[adam-vessey]

Would this presently misreport 404s? As in, attempting to hit the given path for an object which does not exist, would come back as a 403?

[mjordan]

@adam-vessey I just tested, as anonymous (no download privs) using a junk PID and am getting a proper 404.

[DiegoPino]

404 v/s 403 should not be an issue. Menu router still does part of the job right?

[mjordan]

Re. the travis failure, coder-review is passing locally for me:

drush coder-review --reviews=production,security,style,i18n,potx,sniffer islandora_premis
Release standards, Drupal Security Checks, Drupal Coding Standards, Internationalization, Drupal CodeSniffer

sites/all/modules/islandora_premis/islandora_premis.module:
 No Problems Found

sites/all/modules/islandora_premis/includes/admin.form.inc:
 No Problems Found

sites/all/modules/islandora_premis/includes/utilities.inc:
 No Problems Found

sites/all/modules/islandora_premis/islandora_premis.install:
 No Problems Found

sites/all/modules/islandora_premis/tests/islandora_premis.test:
 No Problems Found

Coder found 1 projects, 5 files, 0 warnings were flagged to be ignored          [status]    

[adam-vessey]

@DiegoPino: The it's not really the menu router which handles it, but the delivery callback, which we're overriding...

@mjordan: Ah, looks like the code itself does something unexpected (to me, anyway), handling the 404 on its own...

[mjordan]

@adam-vessey right, that's probably where the 404 is coming from. But as long as the client gets the correct response, does it matter?

[adam-vessey]

@mjordan: Not really, it just caught me off guard.

... I started taking a quick stab at what the latter part of that comment would entail (with eliminating the custom delivery callback), but got hung up with other logic... The way islandora_premis_get_foxml() returns and the like, and what guarantees we want/need to make outside of the module (especially regarding return types and exception handling).

I don't want to hold things up, just highlight that there are some other things in here which could be worth investigating...

[mjordan]

@adam-vessey I totally agree. I'm all for a code review on this module to clean it up a bit along the lines you're suggesting, but after the release. 😁

[mjordan]

@DiegoPino can you tell me how I can find out what the issue is? Runnin drush coder-review --reviews=production,security,style,i18n,potx,sniffer islandora_premis locally does not detect any (see comment above).

[DiegoPino]

Hi, coding standards are being updated constantly, so maybe your phpcs rules are not the latest? But the error message is explicit.

[normal] The first two watchdog() parameters should be literal strings. There should be no variables, concatenation, constants or even a t() call there. Read the documentation.

[mjordan]

Doi, thanks I did not see that explicit message. I'm running a job on an non-Islandora vagrant at the the moment, will fix that and push it up in a bit. Sorry for the trouble.

[DiegoPino]

@mjordan no worries, thanks for the quick response =)

[rosiel]

Phew - now it's just failing cause of the 5.3.3 issue. :)

[DiegoPino]

@mjordan++. Now we need the release one 😀 Great work 🎆