Role settings workbench #104

bseeger · 2021-04-06T19:03:07Z

General information about how to use and test

This document will tell how to setup the access terms and some simple tests you can use as a starting off point:
Role Configuration Document

I tried to write that up as best I could, but please let me know if you get stuck somewhere.

The main portion of this PR is about adding in the roles and access controls for the Collection Level Admin and Global Admin. There are some other minor fixes done along the way (detailed below).

Tickets

Fixes:

Deleting a taxonomy term gives an error page #106 - fixes an error I started seeing yesterday after rebasing off of development. To test this ensure you can delete a term from a vocab like Person or Corporate Body - you should not get an error message and the term should be successfully deleted.
Add "Children" and "Media" tab to the Collection Content Type #35 - adds in Children and Media tabs on a Collections page (when logged in). To test this: log in, create or navigate to a collection page, and ensure that the Children and Member tabs show up.

Addresses:
jhu-idc/iDC-general#227
jhu-idc/iDC-general#220
jhu-idc/iDC-general#226

Partially addresses: jhu-idc/iDC-general#62 - this PR changes the SAML configuration for users that we want to give permission to when they log in (for the local/dev environment only). To test, log in via SSO as staff1 user. They should have the Global Admin role. staff2 should have the collection level admin role upon login.

Creates:
#105
#107
#108
#109

github-actions · 2021-04-06T19:09:13Z

Dependency diff between development base branch development (e6c3630) and PR branch role_settings_workbench (6f6709a):

diff --git a/deps.e6c363014f6ce2f4f7e4866fa6258b2c80c3e9e1 b/deps.pr-104.6f6709a47fcdaebd6e61b105577c2e1513886b4b
index ac4252a..3a7b2d6 100644
--- a/deps.e6c363014f6ce2f4f7e4866fa6258b2c80c3e9e1
+++ b/deps.pr-104.6f6709a47fcdaebd6e61b105577c2e1513886b4b
@@ -39,6 +39,7 @@ doctrine/persistence                                      1.3.8
 doctrine/reflection                                       1.2.2              
 drupal/adaptivetheme                                      3.x-dev 82867ba    
 drupal/admin_toolbar                                      2.3.0              
+drupal/auto_entitylabel                                   3.0.0-beta4        
 drupal/captcha                                            1.1.0              
 drupal/coder                                              8.3.9              
 drupal/config_update                                      1.7.0              
@@ -59,6 +60,7 @@ drupal/devel                                              2.1.0
 drupal/embed                                              1.4.0              
 drupal/entity_browser                                     2.5.0              
 drupal/entity_embed                                       1.1.0              
+drupal/entity_reference_unpublished                       1.2.0              
 drupal/epp                                                1.0.0              
 drupal/eva                                                2.1.0              
 drupal/externalauth                                       1.3.0              
@@ -87,11 +89,15 @@ drupal/rdfui                                              1.0.0-beta3
 drupal/remote_stream_wrapper                              1.5.0              
 drupal/rest_oai_pmh                                       1.0.0-alpha12      
 drupal/restui                                             1.18.0             
+drupal/role_delegation                                    1.1.0              
+drupal/role_hierarchy                                     2.2.0              
 drupal/search_api                                         1.18.0             
 drupal/search_api_solr                                    4.1.4              
 drupal/simplesamlphp_auth                                 3.2.0              
 drupal/token                                              1.9.0              
 drupal/transliterate_filenames                            1.4.0              
+drupal/workbench                                          1.3.0              
+drupal/workbench_access                                   1.0.0-beta4        
 drush/drush                                               9.7.2              
 easyrdf/easyrdf                                           0.9.1              
 egulias/email-validator                                   2.1.25             
@@ -115,10 +121,10 @@ islandora/jsonld                                          dev-8.x-1.x 36f6ca8
 islandora/openseadragon                                   dev-8.x-1.x 1401a03
 jcalderonzumba/gastonjs                                   v1.2.0             
 jcalderonzumba/mink-phantomjs-driver                      v0.3.3             
-jhu-idc/controlled_access_terms                           dev-8.x-1.x bf962a8
+jhu-idc/controlled_access_terms                           dev-8.x-1.x 7a03834
 jhu-idc/idc-ui-theme                                      dev-develop fba85b0
 jhu-idc/idc_ui_module                                     dev-main 9f2a636   
-jhu-idc/islandora_defaults                                dev-8.x-1.x e6a1083
+jhu-idc/islandora_defaults                                dev-8.x-1.x ffbbe98
 jhu-idc/reference_value_pair                              dev-main 192519a   
 jhu_idc/idc_migration                                     v1.0.1             
 justinrainbow/json-schema                                 5.2.10

github-actions · 2021-04-07T20:30:49Z

Dependency diff between development base branch development (8c186ad) and PR branch role_settings_workbench (3b44830):

diff --git a/deps.8c186ad4c5d095e79ad240e40f2cdfaec483040a b/deps.pr-104.3b44830caef26773b761b935d3f4892d4921d855
index 1439eae..ebc6487 100644
--- a/deps.8c186ad4c5d095e79ad240e40f2cdfaec483040a
+++ b/deps.pr-104.3b44830caef26773b761b935d3f4892d4921d855
@@ -39,6 +39,7 @@ doctrine/persistence                                      1.3.8
 doctrine/reflection                                       1.2.2              
 drupal/adaptivetheme                                      3.x-dev 82867ba    
 drupal/admin_toolbar                                      2.3.0              
+drupal/auto_entitylabel                                   3.0.0-beta4        
 drupal/captcha                                            1.1.0              
 drupal/coder                                              8.3.9              
 drupal/config_update                                      1.7.0              
@@ -59,6 +60,7 @@ drupal/devel                                              2.1.0
 drupal/embed                                              1.4.0              
 drupal/entity_browser                                     2.5.0              
 drupal/entity_embed                                       1.1.0              
+drupal/entity_reference_unpublished                       1.2.0              
 drupal/epp                                                1.0.0              
 drupal/eva                                                2.1.0              
 drupal/externalauth                                       1.3.0              
@@ -86,11 +88,15 @@ drupal/rdfui                                              1.0.0-beta3
 drupal/remote_stream_wrapper                              1.5.0              
 drupal/rest_oai_pmh                                       1.0.0-alpha12      
 drupal/restui                                             1.18.0             
-drupal/search_api                                         1.18.0             
+drupal/role_delegation                                    1.1.0              
+drupal/role_hierarchy                                     2.2.0              
+drupal/search_api                                         1.19.0             
 drupal/search_api_solr                                    4.1.4              
 drupal/simplesamlphp_auth                                 3.2.0              
 drupal/token                                              1.9.0              
 drupal/transliterate_filenames                            1.4.0              
+drupal/workbench                                          1.3.0              
+drupal/workbench_access                                   1.0.0-beta4        
 drush/drush                                               9.7.2              
 easyrdf/easyrdf                                           0.9.1              
 egulias/email-validator                                   2.1.25             
@@ -114,10 +120,10 @@ islandora/jsonld                                          dev-8.x-1.x 36f6ca8
 islandora/openseadragon                                   dev-8.x-1.x 1401a03
 jcalderonzumba/gastonjs                                   v1.2.0             
 jcalderonzumba/mink-phantomjs-driver                      v0.3.3             
-jhu-idc/controlled_access_terms                           dev-8.x-1.x bf962a8
+jhu-idc/controlled_access_terms                           dev-8.x-1.x 7a03834
 jhu-idc/idc-ui-theme                                      dev-develop fba85b0
 jhu-idc/idc_ui_module                                     dev-main 9f2a636   
-jhu-idc/islandora_defaults                                dev-8.x-1.x e6a1083
+jhu-idc/islandora_defaults                                dev-8.x-1.x ffbbe98
 jhu-idc/migrate_file                                      1.1.1              
 jhu-idc/reference_value_pair                              dev-main 192519a   
 jhu_idc/idc_migration                                     v1.0.2

birkland · 2021-04-14T17:46:17Z

Minor issue (maybe file new issue): formatting of workbench page is moderately broken in Firefox:

birkland · 2021-04-14T17:47:58Z

codebase/config/sync/simplesamlphp_auth.settings.yml

@@ -8,14 +8,16 @@ login_link_show: true
 login_link_display_name: 'Federated login'
 header_no_cache: false
 role:
-  population: 'staff:eduPersonAffiliation,~=,STAFF'
+  population: 'global_admin:eduPersonPrincipalName,=,[email protected]|collection_level_admin:eduPersonPrincipalName,=,[email protected]'


What worries me is this configuration will be backed into the image that will be deployed in the cloud. In that context: Would this population setting ever be triggered/used in production? i.e. do we depend on certain users (or certain classes of users, e.g. staff) to be assigned roles based on this setting?

If so, would admins for cloud instances (e.g. prod) be expected to set this config value?

If not, then is this 'staff1 = global admin" setting innocuous, as it'll never be triggered outside of the dev environment?

Indeed this would be baked into the cloud image.

Since the principal names are unlikely to be valid in production, the setting is innocuous (no one would ever be able to login to Islandora as 'staff1' or 'staff2').

In production, these values will need to be set to values that are congruent with that environment.

OK, if this needs to be set to something different in prod (or, more generally, different values for different environments,), then we'd probably want an environment variable for it.

Probably a new issue to be dealt with separately? Env vars working with external config is a new thing.

Yeah. I don't have a good understanding of what should be in place for the system in production. We haven't done env vars overriding config before, but it's something that is/will be part of the S3 PR (as it needs to do that).

An example config override in local.settings.php:

$config['s3fs.settings']['bucket'] = getenv('DRUPAL_DEFAULT_S3_BUCKET') ?: 'idc';

This overrides the 'bucket` config value for s3fs.

A few weeks ago I noted some of this here jhu-idc/iDC-general#62, but that ticket doesn't cover the technical way we'd need to work with this.

Sounds like it should be a separate issue because it does require some sort of technical fix.

Should I make a ticket describing all of this?

Yeah, a ticket for figuring out how to override the relevant config (and documenting the environment variables, or whatever is necessary to do so)

jhu-idc/iDC-general#318

birkland · 2021-04-14T19:30:42Z

Before assigning any permissions to staff2, I logged in and looked at workbench. As expected (I think), there was no option to add content (because staff2 hadn't been given access to any collections via access terms yet).

For kicks, I ran a migration as staff2 still before explicitly giving any permissions to staff2, using tests/10-migration-backend-tests/testcafe/migrations/collection-01.csv. Unfortunately (I think), I was able to add random collections that way. I believe this is unexpected. It looks either llike (a) role permissions are not being applied to migrations in this case, or (b) workbench and migrations disagree on permissions to add new collections.

Interestingly, if I navigate to the collections I added via migration, I don't have permissions to edit them

Here is what my workbench looked like after running the migration (note the migration message at the top. Interestingly, it looks like migration success or failure messages don't appear on screen right after you perform a migration, but do appear on the workbench screen once you navigate to it. Is this expected?)

emetsger · 2021-04-14T20:01:16Z

Interestingly, it looks like migration success or failure messages don't appear on screen right after you perform a migration, but do appear on the workbench screen once you navigate to it. Is this expected?)

I think there's an issue open for that here

birkland · 2021-04-14T20:10:51Z

Interestingly, it looks like migration success or failure messages don't appear on screen right after you perform a migration, but do appear on the workbench screen once you navigate to it. Is this expected?)

I think there's an issue open for that here

Yeah, I think that would explain why the migration messages don't appear on the migration page immediately after running it. As far as why the messages do appear on the workbench page is a mystery. Do other sorts of messages appear there? I don't understand the rules at play.

emetsger · 2021-04-14T20:19:28Z

It looks either llike (a) role permissions are not being applied to migrations in this case, or (b) workbench and migrations disagree on permissions to add new collections.

I would not be surprised if migrations did not apply or adhere to role restrictions. (Just like they don't adhere to validation rules that exist on the UI side).

There are some migration plugins which have the ability to "check access", but that seems to be implemented on a per-plugin basis and is not a feature provided by the migration framework as a whole. I'd need to do more research on what "check access" means, vis-a-vis the "user" running the migration if we were interested in pursuing this.

emetsger · 2021-04-14T20:21:14Z

I don't understand the rules at play.

Me either. I think there's what is called a "flash" and for whatever reason the migrate source UI page doesn't display the flash when you're first directed there. I think if you perform a migration, get redirected to migrate source UI page (messages are absent), and then refresh (reloading the migrate source UI page) the messages appear - but I haven't tested that.

birkland · 2021-04-14T20:31:23Z

In testing, I assigned a collection to the access term foo, and gave staff2 permission on foo. staff2 was then allowed to edit the collection (good), but could remove the access term foo from the collection, thus rendering it non-editable after saving. Would this be a consequence of #108, or a new issue entirely?

emetsger

Overall just some comments on a couple issues relating to eliding comments by formatting, and the use of get from getSingle in some tests.

codebase/config/sync/migrate_plus.migration.idc_ingest_new_items.yml

tests/10-migration-backend-tests/verification/verify_migrations_test.go

birkland · 2021-04-14T21:23:58Z

Openseadragon does not work for anonymous users.

I can create an Image repository item as staff1, add media, and see the viewer just fine. However, logging out and viewing it anonymously results in a black, blank area where the viewer should be.

birkland

This looks quite great, but is a bit too huge to review comprehensively. Right now, the only thing I see that explicitly represents a functionality regression is that openseadragon seems to be broken for anonymous users. That might be a quick fix with permissions tweaks 🤞. If not, then we could probably talk about merging with that as a known issue.

Otherwise, this looks good, but I don't feel I was able to fully assess its impact manually. This is really deserving of a test suite (as mentioned in the test plan testing various expected permissions and denials based on roles). It's probably worth stakeholder review before that happens, though, as this PR is be impactful on user experience.

Aside from the tests, this work is functionally incomplete in a few significant areas

member_of permissions are not checked, effectively allowing anybody to add items to any collection (already captured in tickets, e.g. create presave validation hook for field_member_of and field_access_terms #108)
roles/permissions do not seem to be applied to migrations (not ticketed yet, I believe)

... but implementing roles and permissions globally a huge far-reaching chunk of work that deserves being split up into more manageable chunks. This PR appropriately implements the core of it, and demonstrates the fundamental paradigm well.

Please see earlier comments for suggestions for new issues/tickets to address down the road.

bseeger · 2021-04-16T11:58:25Z

In testing, I assigned a collection to the access term foo, and gave staff2 permission on foo. staff2 was then allowed to edit the collection (good), but could remove the access term foo from the collection, thus rendering it non-editable after saving. Would this be a consequence of #108, or a new issue entirely?

It could be considered a new issue, depending on whether or not we think a user should be able to edit the access terms on a node. If they can access it they they'll be able to remove their own access to it. If they can't access it, then we can add some sort of check as part of #108 to ensure that that field hasn't changed. The nice thing is that field is limited to the collections (aka workbench sections) the user has access to, and if a CLA did want to move an object around from one collection to another, they would have to have access to this field.

bseeger · 2021-04-16T15:51:16Z

@birkland

Openseadragon does not work for anonymous users.

I spoke with @jaredgalanis about this and he's seen this error before elsewhere. He's going to take a look at this and file a ticket for it. We don't think that this is created by this PR as he's seen it before.

bseeger · 2021-04-16T16:02:27Z

Formatting ticket for looking at that workbench UI overlap: jhu-idc/iDC-general#319

bseeger · 2021-04-19T14:18:51Z

roles/permissions do not seem to be applied to migrations (not ticketed yet, I believe)

I created #114 to track the above.

And for the issue of whether or not a user should be able to change access_terms, I created this ticket: #115

Minor tweaks to other things.

controlled_access_term module commits.

Minor tweaks to other things.

upgrades search_api to 1.19 to fix a bug we were seeing fixes the corporate bodies csv test file to include a required field.

github-actions · 2021-04-19T18:47:43Z

Dependency diff between development base branch development (94340a0) and PR branch role_settings_workbench (35380af):

diff --git a/deps.94340a04f7a2b54964ad67f0f9860ab473d6176d b/deps.pr-104.35380af0d832027ff926b0121447e4ba7c1c1536
index cbd2a47..25fda75 100644
--- a/deps.94340a04f7a2b54964ad67f0f9860ab473d6176d
+++ b/deps.pr-104.35380af0d832027ff926b0121447e4ba7c1c1536
@@ -39,6 +39,7 @@ doctrine/persistence                                      1.3.8
 doctrine/reflection                                       1.2.2              
 drupal/adaptivetheme                                      3.x-dev 82867ba    
 drupal/admin_toolbar                                      2.3.0              
+drupal/auto_entitylabel                                   3.0.0-beta4        
 drupal/captcha                                            1.1.0              
 drupal/coder                                              8.3.9              
 drupal/config_update                                      1.7.0              
@@ -59,6 +60,7 @@ drupal/devel                                              2.1.0
 drupal/embed                                              1.4.0              
 drupal/entity_browser                                     2.5.0              
 drupal/entity_embed                                       1.1.0              
+drupal/entity_reference_unpublished                       1.2.0              
 drupal/epp                                                1.0.0              
 drupal/eva                                                2.1.0              
 drupal/externalauth                                       1.3.0              
@@ -86,11 +88,15 @@ drupal/rdfui                                              1.0.0-beta3
 drupal/remote_stream_wrapper                              1.5.0              
 drupal/rest_oai_pmh                                       1.0.0-alpha12      
 drupal/restui                                             1.18.0             
-drupal/search_api                                         1.18.0             
+drupal/role_delegation                                    1.1.0              
+drupal/role_hierarchy                                     2.2.0              
+drupal/search_api                                         1.19.0             
 drupal/search_api_solr                                    4.1.4              
 drupal/simplesamlphp_auth                                 3.2.0              
 drupal/token                                              1.9.0              
 drupal/transliterate_filenames                            1.4.0              
+drupal/workbench                                          1.3.0              
+drupal/workbench_access                                   1.0.0-beta4        
 drush/drush                                               9.7.2              
 easyrdf/easyrdf                                           0.9.1              
 egulias/email-validator                                   2.1.25             
@@ -114,10 +120,10 @@ islandora/jsonld                                          dev-8.x-1.x 36f6ca8
 islandora/openseadragon                                   dev-8.x-1.x 1401a03
 jcalderonzumba/gastonjs                                   v1.2.0             
 jcalderonzumba/mink-phantomjs-driver                      v0.3.3             
-jhu-idc/controlled_access_terms                           dev-8.x-1.x bf962a8
+jhu-idc/controlled_access_terms                           dev-8.x-1.x 7a03834
 jhu-idc/idc-ui-theme                                      dev-develop 3e46bfa
 jhu-idc/idc_ui_module                                     dev-main a554904   
-jhu-idc/islandora_defaults                                dev-8.x-1.x e6a1083
+jhu-idc/islandora_defaults                                dev-8.x-1.x ffbbe98
 jhu-idc/migrate_file                                      1.1.1              
 jhu-idc/reference_value_pair                              dev-main 192519a   
 jhu_idc/idc_migration                                     v1.0.2

birkland

I think all the issues that needed to be created, are. looks good.

This was referenced Apr 7, 2021

Deleting a taxonomy term gives an error page #106

Closed

Assign roles to SAML users jhu-idc/iDC-general#62

Closed

bseeger force-pushed the role_settings_workbench branch from 8329fbd to 3b44830 Compare April 7, 2021 20:11

bseeger marked this pull request as ready for review April 7, 2021 20:27

bseeger requested review from birkland and emetsger April 7, 2021 20:28

This was referenced Apr 7, 2021

Add field_access_terms to media migrations & media tests #110

Closed

Add "Children" and "Media" tab to the Collection Content Type #35

Closed

birkland reviewed Apr 14, 2021

View reviewed changes

emetsger reviewed Apr 14, 2021

View reviewed changes

birkland suggested changes Apr 15, 2021

View reviewed changes

Bethany Seeger added 5 commits April 19, 2021 11:02

first pass at adding collection level admin and global admin permissions

7b4a382

Adding env

1fb573e

Latest changes, still a WIP

7169073

Almost there. Tweaks a bunch of small things.

e76ecc9

More tweaks

66a5f65

Bethany Seeger added 17 commits April 19, 2021 13:26

Removed bad file.

96bce7e

Adds tests. They are a WIP though.

982457c

Fixed the migration tests.

7fc8696

Minor tweaks to other things.

Creates snapshot and updates composer to latests islandora_defaults and

5cfaeea

controlled_access_term module commits.

first pass at adding collection level admin and global admin permissions

6f12bed

Adding env

4665c3f

Latest changes, still a WIP

83a36a1

Almost there. Tweaks a bunch of small things.

aafe435

More tweaks

a0603bb

Removed bad file.

8919172

Adds tests. They are a WIP though.

2a90cd6

Fixed the migration tests.

4878161

Minor tweaks to other things.

Updates snapshots

66b6665

upgrades search_api to 1.19 to fix a bug we were seeing fixes the corporate bodies csv test file to include a required field.

Updates the snapshot image info in env.

4c8dc61

Changes call to get to getSingle in a few spots.

3f7e2b6

Reverts formatting changes that were unnecessary.

0145185

Updates snapshot for dev changes.

35380af

bseeger force-pushed the role_settings_workbench branch from 78e515e to 35380af Compare April 19, 2021 18:42

birkland approved these changes Apr 19, 2021

View reviewed changes

bseeger merged commit 22d0e22 into development Apr 20, 2021

This was referenced Apr 20, 2021

ensure that collection can be editted by CA and GAs jhu-idc/iDC-general#227

Closed

Configure Collection Admin role jhu-idc/iDC-general#220

Closed

configure Global Admin role jhu-idc/iDC-general#226

Closed

bseeger deleted the role_settings_workbench branch April 20, 2021 12:23

bseeger mentioned this pull request Apr 26, 2021

ensure that collection can be edited by CA and GAs jhu-idc/iDC-general#221

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Role settings workbench #104

Role settings workbench #104

bseeger commented Apr 6, 2021 •

edited

Loading

github-actions bot commented Apr 6, 2021

github-actions bot commented Apr 7, 2021

birkland commented Apr 14, 2021

birkland Apr 14, 2021

emetsger Apr 14, 2021

birkland Apr 14, 2021

emetsger Apr 14, 2021

birkland Apr 15, 2021

bseeger Apr 16, 2021

birkland Apr 16, 2021

bseeger Apr 16, 2021

birkland commented Apr 14, 2021 •

edited

Loading

emetsger commented Apr 14, 2021

birkland commented Apr 14, 2021

emetsger commented Apr 14, 2021

emetsger commented Apr 14, 2021

birkland commented Apr 14, 2021

emetsger left a comment

birkland commented Apr 14, 2021

birkland left a comment

bseeger commented Apr 16, 2021 •

edited

Loading

bseeger commented Apr 16, 2021 •

edited

Loading

bseeger commented Apr 16, 2021

bseeger commented Apr 19, 2021

github-actions bot commented Apr 19, 2021

birkland left a comment

Role settings workbench #104

Role settings workbench #104

Conversation

bseeger commented Apr 6, 2021 • edited Loading

General information about how to use and test

Tickets

github-actions bot commented Apr 6, 2021

github-actions bot commented Apr 7, 2021

birkland commented Apr 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

birkland commented Apr 14, 2021 • edited Loading

emetsger commented Apr 14, 2021

birkland commented Apr 14, 2021

emetsger commented Apr 14, 2021

emetsger commented Apr 14, 2021

birkland commented Apr 14, 2021

emetsger left a comment

Choose a reason for hiding this comment

birkland commented Apr 14, 2021

birkland left a comment

Choose a reason for hiding this comment

bseeger commented Apr 16, 2021 • edited Loading

bseeger commented Apr 16, 2021 • edited Loading

bseeger commented Apr 16, 2021

bseeger commented Apr 19, 2021

github-actions bot commented Apr 19, 2021

birkland left a comment

Choose a reason for hiding this comment

bseeger commented Apr 6, 2021 •

edited

Loading

birkland commented Apr 14, 2021 •

edited

Loading

bseeger commented Apr 16, 2021 •

edited

Loading

bseeger commented Apr 16, 2021 •

edited

Loading