embargo roles whitelist #232

qadan · 2016-04-20T20:16:47Z

JIRA Ticket: https://jira.duraspace.org/browse/ISLANDORA-1602

Other Relevant Links: Enables roles with "can embargo any object" to manage embargoed items #220 (the original pull request for this issue)

What does this Pull Request do?

Allows a roles whitelist to be configured and applied to embargoes.

What's new?

Three things:

A configurable whitelist has been added to establish trusted roles with the 'can embargo any' permission that are added to embargoes when they are set via islandora_scholar_embargo_set_embargo().
A batch has been added to reapply existing embargoes. It can be triggered via the configurable whitelist form, if desired.
A drush command has been added, islandora_scholar_embargo_reapply_embargoes, which also triggers the batch re-application.

How should this be tested?

The whitelist form should be tested to make sure the variables it sets take the form of an associative array pairing role IDs with role names.
The batch should be tested to ensure embargoes on objects and datastreams are applied properly
The drush command should also be tested to ensure it functions as expected.

As this has XACML security implications, we should be ensuring that when an embargo is created, a POLICY is applied that only contains the roles in the whitelist. POLICYs for entire objects should have viewing and management permissions for whitelisted roles.

Additional Notes:

Care has been taken to structure the descriptions of things so that there's no ambiguity, since ambiguous and misleading documentation seems to be part of the source of this issue. Documentation within the module should probably be checked to make sure it's internally consistent and communicates clearly.

One of the things that is documented in this pull but bears repeating is that due to the way the Fedora-Drupal filter works, matching role IDs between sites that share the same Fedora represent a method of seeing objects that would otherwise be hidden. Whitelist roles should likely be unique and/or namespaced to mitigate this issue.

Interested parties

From the original PR: @bryjbrown @rosiel @jordandukart

…ll jam an extra letter into the pluralization

adam-vessey · 2016-04-21T18:23:04Z

modules/islandora_scholar_embargo/includes/admin.form.inc

-    $options = array($cmodel => $options[$cmodel]) + $options;
+    if (isset($options[$cmodel])) {
+      $options = array($cmodel => $options[$cmodel]) + $options;
+    }


I'm... not sure I understand what's going on here. If there's an entry in $options, we reset its value to itself?

Are we looking for something more like:

$options = array_intersect_key($options, drupal_map_assoc($selected));

instead of this foreach over $selected?

... Actually... This was some craziness to establish the "desired" ordering or something, was it?

It's just because it's possible for those options not to be returned in islandora_get_content_models() due to XACML or other what-have-ye's, so $options[$cmodel] may not exist. But yeah, there's probably a more PHP-weird-array-business way to do it?

Yeah, it wasn't so much the isset(), but the whole: "options = one of it's items + the rest of it"... Which would only have the affect of (re)ordering the array...?

Perhaps the intention was to place the current config at the top? Because ... UI design? I dunno?

Shrug. Is fine.

adam-vessey · 2016-04-22T19:04:56Z

modules/islandora_scholar_embargo/includes/embargo.inc

+  { ?obj <fedora-view:lastModifiedDate> ?date ;
+         <fedora-view:disseminationType> ?dsid
+  }
+  FILTER(?date > "$offset"^^<http://www.w3.org/2001/XMLSchema#dateTime>)


Could miss items, two (or more) items happen to have the same date (and the set crosses a slice boundary). Would have to be something like "date greater or (date equal and something to break the tie)"... Usually using the PID...

PREFIX is: <info:islandora/islandora-system:def/scholar#> SELECT DISTINCT ?obj ?expiry ?date FROM <#ri> WHERE { ?obj is:embargo-until ?expiry ; { ?obj <fedora-model:createdDate> ?date } UNION { ?obj <fedora-view:lastModifiedDate> ?date ; <fedora-view:disseminationType> ?dsid } FILTER(?date > "$offset"^^<http://www.w3.org/2001/XMLSchema#dateTime> || (?date = "$offset"^^<http://www.w3.org/2001/XMLSchema#dateTime> && ?obj > "$the_last_one")) } ORDER BY ASC(?date), ASC(?obj)

or something of the like.

ruebot · 2016-04-24T20:45:57Z

@qadan does this supercede #220? If so, do you need @bryjbrown to close his PR? Or did you want to cherry-pick his work into this?

qadan · 2016-04-25T12:54:21Z

It'll likely be the former; I don't think #220 will correctly cherry-pick into this one. I was going to wait on resolution of this ticket before taking care of the ticket cleanup

ruebot · 2016-04-25T12:57:36Z

@qadan might be a good idea to let @bryjbrown know what exactly what is going on since he is a new contributor.

qadan · 2016-04-25T13:51:52Z

@ruebot yep; that conversation actually happened in https://jira.duraspace.org/browse/ISLANDORA-1602! I asked him if I could take over the work before starting on this.

ruebot · 2016-04-25T13:53:52Z

@qadan that reads as if you were offering to do a pull request against @bryjbrown's pull request. Not creating a whole new pull request. At least that is my reading of the conversation.

qadan · 2016-04-25T14:01:08Z

That was indeed the original plan, but I attempted to indicate in the last comment on there that the plan changed due to a change in the method taken and the number of things that had to be altered; sorry if that wasn't clear.

adam-vessey · 2016-04-25T16:47:00Z

modules/islandora_scholar_embargo/includes/embargo.inc

+    "FILTER(?date > \"$offset_date\"^^<http://www.w3.org/2001/XMLSchema#dateTime>)" :
+    "FILTER(?date > \"$offset_date\"^^<http://www.w3.org/2001/XMLSchema#dateTime> || (?date = \"$offset_date\"^^<http://www.w3.org/2001/XMLSchema#dateTime> && str(?obj) > \"$offset_pid\"))";
+  $order_by = is_null($offset_pid) ?
+    "ORDER BY ASC(?date)" :


This would still have to order ASC(?obj), to ensure we can pass correctly to the second slice. As in, still: ORDER BY ASC(?date), ASC(?obj).

Thought could save some time on the ordering, but that makes sense. Repair'd

adam-vessey · 2016-04-25T16:49:59Z

modules/islandora_scholar_embargo/includes/embargo.inc

+function islandora_scholar_embargo_get_embargoed_items_subset($offset_date = '1900-01-01T00:00:00.000Z', $offset_pid = NULL, $limit = 10) {
+  // Establish some parameters for the query.
+  $filter = is_null($offset_pid) ?
+    "FILTER(?date > \"$offset_date\"^^<http://www.w3.org/2001/XMLSchema#dateTime>)" :


Could we not just avoid specifying the filter entirely, until we have a value based on data? Just use the empty string, and have things be in the proper order. We'll grab the first slice of $limit items, and next time we get called, we'll have the two values at which to start the next slice.

ruebot · 2016-05-12T17:24:49Z

@qadan is there a release branch pull request for this, since it is listed as a bug in JIRA?

qadan · 2016-05-12T17:33:00Z

@ruebot I honestly had not realized that it was listed as a bug! I'd assumed throughout that this was an improvement. I don't know, personally, if I'd consider this to be a bug? Looking at the former discussion on this, it seems to waver somewhere between "is a bug" and "is working as intended but is misleading"

ruebot · 2016-05-12T17:33:49Z

@qadan sounds like a good discussion for the Committers Call then 😉

qadan · 2016-05-12T17:34:26Z

Definitely will bring up; there's either two or three 7.x-1.7 pulls left to make depending on how this would be considered.

nigelgbanks · 2016-05-23T10:52:07Z

modules/islandora_scholar_embargo/islandora_scholar_embargo.module

@@ -502,14 +513,15 @@ function islandora_scholar_embargo_set_embargo($param, $dsid = NULL, $end = 'ind
          $embargo_ds->relationships->add(ISLANDORA_SCHOLAR_EMBARGO_RELS_URI, ISLANDORA_SCHOLAR_EMBARGO_NOTIFICATION_PRED, $notification, $type);
        }
        $xacml->datastreamRule->addDsid($datastream_id);
-        $xacml_changed = TRUE;
        foreach ($users as $user) {


We need another foreach here to apply the roles as is done above.

foreach ($roles as $role) { $xacml->datastreamRule->addRole($role); }

nigelgbanks · 2016-05-23T10:52:35Z

Looks like we need some additional work? Newly embargoed objects don't inherit the whitelist. This only applies when embargoing data-streams, embargoing the whole object works.

Steps to reproduce.

Create new role 'test'.
Add permission 'Manage embargo on any objects'.
Add new role 'test' to the whitelist here admin/islandora/solution_pack_config/embargo/roles
Embargo and existing object that currently does not have an embargo set (You must choose data-steam based embargo).
View the XACML data-stream and note the permissions.

See the comment here for a fix: https://github.com/Islandora/islandora_scholar/pull/232/files#r64203012

@qadan I've sent you a pull with the roles added.

https://github.com/qadan/islandora_scholar/pull/1/files

Add whitelisted roles when embargoing a data-stream.

ruebot · 2016-05-24T13:42:50Z

@nigelgbanks why are you merging a pull request that you have a commit in?

qadan · 2016-05-24T14:24:05Z

@ruebot true; that is covered under 5.iii of the committers workflow. Would it have made more sense to reject the PR and implement the line myself? I'm personally fine with it as-is, especially given the 'PR-as-code-review-recommendation' nature of the commit, and given the discussion surrounding this ticket on the last committers call regarding the general unfamiliarity with Scholar; @nigelgbanks was the only one the group could find outside of DGI willing to do the review/merge

ruebot · 2016-05-24T14:27:17Z

@qadan it should not have been merged as it was, full stop. That is in direct violation of the process we spent months working on. But, I guess it is what it is, and this y'all's community now. I'm out 😉

manez · 2016-05-24T14:29:37Z

+1 for this being within the spirit of the law, if not quite the letter. As long as we've got clear communication around what's happening (and I think we do here), I don't see the benefit in backing this one out and redoing it.

While noting that yes, it was technically a self-commit and shouldn't have gone in this way.

bryjbrown · 2016-05-31T19:38:45Z

Looks like its a bit too late to be helpful since the pull has already been merged, but I did complete the smoke test for this pull on an Islandora Vagrant machine today. To summarize the more fleshed out comment in JIRA, it works but seems to throw a nontrivial amount of errors and the secondary layer of permissions created by the whitelist might be a bit confusing to users (although links to the whitelist in the permission description do help). The batch embargo updater is great when it works, but seems to throw a 500 AJAX error roughly half the time.

whikloj · 2016-06-01T19:18:53Z

@bryjbrown, I'd suggest a new JIRA ticket to fix this new feature and get it into a working state.

bryjbrown · 2016-06-01T20:15:28Z

@whikloj Someone else is free to carry the torch on this if they like, otherwise I'm just going to let it be.
I'm not very heartened by the path this has taken (PR rejected, obvious bug changed to a new feature request in JIRA, completely new solution designed without input from community, etc).

We (FSU/FLVC) resolved the issue to our own satisfaction with the pull request included in [my original ticket (JIRA 1602)|https://jira.duraspace.org/browse/ISLANDORA-1602], and are just going to stay on our FLVC fork of Scholar for the time being instead of updating. What's left of this issue should be pushed forward by someone with more skin in the game.

Also, apologies if this comes across as rude to anyone, particularly anyone who made commits on this. I appreciate the work thats gone into this & thank you for it, and I'll take the blame on this for stalling on the ticket when it was assigned to me back in April and misunderstanding the process that followed. I'll try to be more active in the process next time to scratch my own itches. 😄

qadan · 2016-06-01T21:18:52Z

@bryjbrown Not rude, I think! It's probably far more healthy to get your concerns out in the open rather than otherwise.

To explain the decision behind the whitelist and the dual-permissions thing: one deals with Drupal permisssions and the other deals with XACML permissions. While I agree it mostly seems like a secondary layer, precedent so far has exclusively leaned towards not using Drupal permissions to dictate XACML permissions, and I didn't really see a pressing enough need to deviate from that precedent.

The bug-to-feature change was done on a committer's call; I think it boiled down to the above precedent again - Drupal permissions don't currently dictate XACML permissions anywhere, and instead so far we've opted to document this fact on the Drupal permissions page anywhere XACML could override Drupal, so the documentation was updated.

As to the last point, though ... I'm honestly not sure what the better path was here for input from the community? This pull was opened April 20th and closed over a month later, I believe? It was brought up a couple times in committer's, and I pinged the interested parties on here at least a couple times before it got in as well, but I just don't know what else I can do beyond these types of things. The last committer's call this was brought up in, I couldn't find anyone willing to review it besides @nigelgbanks (which, thanks Nigel!) - somewhat understandable, since this is pretty niche functionality, but honestly more input would be fantastic! Especially when there's concerns like the one with the whitelist. Just ... I dunno, over a month later, well past its original review, I'm not sure what other means are available to me.

If you feel like the original PR was squashed, that's my bad; I think muscle memory just makes me "WRITE CODE MAKE PR" haha, and I probably could have PR'd my additions against yours. I wish I'd known you'd felt as such; I could have just closed this one and done that instead.

And as for the bugs found, I'll take a peek soon! If I've submitted something broken, I should probably clean up my mess >_>

bryjbrown · 2016-06-02T10:01:11Z

@qadan That's good to know that the reason for the whitelist had to do with staying consistent with a previous precedent. While I don't necessarily agree with the decision (but would be open to discussion about it), I can at least appreciate the logic behind it.

I think in the future though, we should try to keep all discussion of the problem under one JIRA ticket to make it linear and easy to follow. The jump from JIRA 1602 to JIRA 1717 is never made explicit, and this is confusing for people on the outside looking in (such as @DonRichards in this comment and trying to follow the thread from bug description through discussion of design and implementation. Having to skip back and forth between multiple tickets adds more complexity and should be avoided if possible.

All in all I think I was just having a rough afternoon yesterday and was already feeling pretty salty, and @whikloj's suggestion about creating a third ticket prompted me to post a fairly tactless comment (I know this was not his intention of course). I appreciate @qadan addressing my concerns about this issue as well, hearing his side of the story puts things in a better perspective. 👍

qadan · 2016-06-02T10:56:19Z

@bryjbrown definitely agree; this one got a little ... weird. I think a lot of that's on me. I'll try to keep things nice and clean in the future. And I'll see about those errors too; seems kinda troubling

qadan added 4 commits April 20, 2016 16:54

embargo roles whitelist

1f9eae7

that's totally a valid spelling BUT for consistency's sake, FINE, we'…

bfc85f2

…ll jam an extra letter into the pluralization

from an earlier idea; forgot to remove this

b90ac10

same type deal

ce16056

adam-vessey reviewed Apr 21, 2016
View reviewed changes

recommended fixes

ec41b8f

adam-vessey reviewed Apr 22, 2016
View reviewed changes

offsetting by pid too. also getting that var del in there :/

420aa12

adam-vessey reviewed Apr 25, 2016
View reviewed changes

always asc those objs

ba4026a

adam-vessey reviewed Apr 25, 2016
View reviewed changes

qadan added 2 commits April 25, 2016 15:55

perfecting the query

cb20e6d

slight misunderstanding of the way #checkboxes work

8070e7e

qadan mentioned this pull request May 12, 2016

documenting XACML #235

Merged

nigelgbanks reviewed May 23, 2016
View reviewed changes

nigelgbanks and others added 2 commits May 23, 2016 12:08

Add whitelisted roles when embargoing a data-stream.

8dcf23c

Merge pull request #1 from nigelgbanks/7.x-embargo-roles

35a2baf

Add whitelisted roles when embargoing a data-stream.

nigelgbanks merged commit 25a8b1a into Islandora:7.x May 24, 2016

bryjbrown mentioned this pull request Jun 7, 2016

Adds info:islandora/islandora-system:def/scholar# predicates to both … Islandora/islandora_ontology#5

Closed

bryjbrown mentioned this pull request Sep 14, 2016

Enables roles with "can embargo any object" to manage embargoed items #220

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

embargo roles whitelist #232

embargo roles whitelist #232

qadan commented Apr 20, 2016 •

edited

Loading

adam-vessey Apr 21, 2016

qadan Apr 21, 2016 •

edited

Loading

adam-vessey Apr 21, 2016

qadan Apr 21, 2016

adam-vessey Apr 22, 2016

adam-vessey Apr 22, 2016 •

edited

Loading

ruebot commented Apr 24, 2016

qadan commented Apr 25, 2016

ruebot commented Apr 25, 2016

qadan commented Apr 25, 2016

ruebot commented Apr 25, 2016

qadan commented Apr 25, 2016

adam-vessey Apr 25, 2016 •

edited

Loading

qadan Apr 25, 2016

adam-vessey Apr 25, 2016

ruebot commented May 12, 2016

qadan commented May 12, 2016

ruebot commented May 12, 2016

qadan commented May 12, 2016

nigelgbanks May 23, 2016 •

edited

Loading

nigelgbanks commented May 23, 2016 •

edited

Loading

ruebot commented May 24, 2016

qadan commented May 24, 2016

ruebot commented May 24, 2016

manez commented May 24, 2016

bryjbrown commented May 31, 2016 •

edited

Loading

whikloj commented Jun 1, 2016

bryjbrown commented Jun 1, 2016

qadan commented Jun 1, 2016

bryjbrown commented Jun 2, 2016 •

edited

Loading

qadan commented Jun 2, 2016

embargo roles whitelist #232

embargo roles whitelist #232

Conversation

qadan commented Apr 20, 2016 • edited Loading

What does this Pull Request do?

What's new?

How should this be tested?

Additional Notes:

Interested parties

adam-vessey Apr 21, 2016

Choose a reason for hiding this comment

qadan Apr 21, 2016 • edited Loading

Choose a reason for hiding this comment

adam-vessey Apr 21, 2016

Choose a reason for hiding this comment

qadan Apr 21, 2016

Choose a reason for hiding this comment

adam-vessey Apr 22, 2016

Choose a reason for hiding this comment

adam-vessey Apr 22, 2016 • edited Loading

Choose a reason for hiding this comment

ruebot commented Apr 24, 2016

qadan commented Apr 25, 2016

ruebot commented Apr 25, 2016

qadan commented Apr 25, 2016

ruebot commented Apr 25, 2016

qadan commented Apr 25, 2016

adam-vessey Apr 25, 2016 • edited Loading

Choose a reason for hiding this comment

qadan Apr 25, 2016

Choose a reason for hiding this comment

adam-vessey Apr 25, 2016

Choose a reason for hiding this comment

ruebot commented May 12, 2016

qadan commented May 12, 2016

ruebot commented May 12, 2016

qadan commented May 12, 2016

nigelgbanks May 23, 2016 • edited Loading

Choose a reason for hiding this comment

nigelgbanks commented May 23, 2016 • edited Loading

Steps to reproduce.

ruebot commented May 24, 2016

qadan commented May 24, 2016

ruebot commented May 24, 2016

manez commented May 24, 2016

bryjbrown commented May 31, 2016 • edited Loading

whikloj commented Jun 1, 2016

bryjbrown commented Jun 1, 2016

qadan commented Jun 1, 2016

bryjbrown commented Jun 2, 2016 • edited Loading

qadan commented Jun 2, 2016

qadan commented Apr 20, 2016 •

edited

Loading

qadan Apr 21, 2016 •

edited

Loading

adam-vessey Apr 22, 2016 •

edited

Loading

adam-vessey Apr 25, 2016 •

edited

Loading

nigelgbanks May 23, 2016 •

edited

Loading

nigelgbanks commented May 23, 2016 •

edited

Loading

bryjbrown commented May 31, 2016 •

edited

Loading

bryjbrown commented Jun 2, 2016 •

edited

Loading