ualbertalib / HydraNorth

This repo is deprecated. Succeeded by https://github.com/ualbertalib/jupiter. This codebase was a IR built based on Samvera/Sufia
11 stars 4 forks source link

CCID-authenticated items #582

Closed pbinkley closed 8 years ago

pbinkley commented 9 years ago

Based on SAML work: items with CCID-protected status should only be accessible to users who have authenticated via SAML. This is what defines a ualberta user.

mbarnett commented 9 years ago

Is the desire here to require CCID for all items with a visibility setting of Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_AUTHENTICATED, or is the desire to create a new level of authentication, with VISIBILITY_TEXT_VALUE_AUTHENTICATED available to all authenticated users, and the new visibility level restricted to CCID?

weiweishi commented 9 years ago

Sufia's assumption is based on all registered users have Penn State's campus ID as the application was designed to link with their LDAP. They don't allow "public but registered" user and only allow "users from the institution".

But we ought to distinguish a registered user and a ccid user. Since Hydra's AccessControls only provides three levels of visibility (https://github.com/projecthydra/hydra-head/blob/master/hydra-access-controls/app/models/concerns/hydra/access_controls/visibility.rb) , we may have to interpret the new visibility and map to one of the three levels.

Do you want to have a chat and map it out on a whiteboard? @pbinkley @pgwillia

mbarnett commented 9 years ago

Currently we treat "Authenticated" visibility as synonymous with "U of A user" in a lot of places in the interface eg) https://github.com/ualbertalib/HydraNorth/blob/master/app/helpers/application_helper.rb#L5

so that will also have to change if we want to distinguish 'just authenticated' from 'authenticated specifically by CCID'

mbarnett commented 9 years ago

Ok, this doesn't look as bad as I thought it might be. The basic plan of attack is to:

  1. Create a new HydraNorth::AccessControls::Visibility module, extending Hydra::AccessControls::Visibility.
  2. Define new visibility states on top of Hydra's default states. Hydra's visibility states map to group names ('public', 'authenticated', etc) stored on the object, so the new states will be defined by looping over a set of institution names we will define in HydraNorth's config. These institution names will be those institutions that hydranorth provides repository services for. For our purposes, this will mean that objects will have a visibility value of 'university_of_alberta', if visibility is restricted to only members of this institution.
  3. include this in our base GenericFIle model, overriding the default Hydra visibility logic
  4. add logic to HydraNorth::Ability#custom_permissions to give a user cancan access rights to the show page of an object with an institutional visibility restriction iff the user is defined as belonging to that institution (which, for us, means they're CCID authenticated)
  5. modify the UI in various places (permissions edit, etc) to not label purely requiring authentication as being UofA specific, and to add the institutionally specific visibility option for the UofA.

Per the earlier conversation, CCID-restricted objects will still appear on index pages. Only the show page will require CCID authentication to view.

This should set us up fairly generically for extending repository service to other institutions, with only bits of the view that are UofA specific, and the auth logic that currently lives in the User model needing to be re-factored.

Based on the above plan I think I could have this done sometime Thursday, so it's probably 1-2 days easier than my earlier guess. Resizing to medium.

weiweishi commented 9 years ago

Sounds great. Thanks @mbarnett

Weiwei Shi

Digital Initiative Applications Librarian University of Alberta Libraries 2-10L Cameron Library Edmonton, Alberta, Canada T6G 2J8 Phone:(780)492-7802 Fax: (780)248-1209 Email: weiwei.shi@ualberta.ca

On Tue, Sep 15, 2015 at 3:59 PM, Matthew Barnett notifications@github.com wrote:

Ok, this doesn't look as bad as I thought it might be. The basic plan of attack is to:

  1. Create a new HydraNorth::AccessControls::Visibility module, extending Hydra::AccessControls::Visibility.
  2. Define new visibility states on top of Hydra's default states. Hydra's visibility states map to group names ('public', 'authenticated', etc) stored on the object, so the new states will be defined by looping over a set of institution names we will define in HydraNorth's config. These institution names will be those institutions that hydranorth provides repository services for. For our purposes, this will mean that objects will have a visibility value of 'university_of_alberta', if visibility is restricted to only members of this institution.
  3. include this in our base GenericFIle model, overriding the default Hydra visibility logic
  4. add logic to HydraNorth::Ability#custom_permissions to give a user cancan access rights to the show page of an object with an institutional visibility restriction iff the user is defined as belonging to that institution (which, for us, means they're CCID authenticated)
  5. modify the UI in various places (permissions edit, etc) to not label purely requiring authentication as being UofA specific, and to add the institutionally specific visibility option for the UofA.

Per the earlier conversation, CCID-restricted objects will still appear on index pages. Only the show page will require CCID authentication to view.

This should set us up fairly generically for extending repository service to other institutions, with only bits of the view that are UofA specific, and the auth logic that currently lives in the User model needing to be re-factored.

Based on the above plan I think I could have this done sometime Thursday, so it's probably 1-2 days easier than my earlier guess. Resizing to medium.

— Reply to this email directly or view it on GitHub https://github.com/ualbertalib/HydraNorth/issues/582#issuecomment-140558681 .

sfarnel commented 9 years ago

quick question. in current ERA, metadata - both brief record in result list and full record on item page - is visible for ccid protected items; users have to authenticate only to download the object. if i read the above right, only a brief metadata record will be visible, not the full record? sorry if i'm missing something.

On Tue, Sep 15, 2015 at 4:03 PM, Weiwei Shi notifications@github.com wrote:

Sounds great. Thanks @mbarnett

Weiwei Shi

Digital Initiative Applications Librarian University of Alberta Libraries 2-10L Cameron Library Edmonton, Alberta, Canada T6G 2J8 Phone:(780)492-7802 Fax: (780)248-1209 Email: weiwei.shi@ualberta.ca

On Tue, Sep 15, 2015 at 3:59 PM, Matthew Barnett <notifications@github.com

wrote:

Ok, this doesn't look as bad as I thought it might be. The basic plan of attack is to:

  1. Create a new HydraNorth::AccessControls::Visibility module, extending Hydra::AccessControls::Visibility.
  2. Define new visibility states on top of Hydra's default states. Hydra's visibility states map to group names ('public', 'authenticated', etc) stored on the object, so the new states will be defined by looping over a set of institution names we will define in HydraNorth's config. These institution names will be those institutions that hydranorth provides repository services for. For our purposes, this will mean that objects will have a visibility value of 'university_of_alberta', if visibility is restricted to only members of this institution.
  3. include this in our base GenericFIle model, overriding the default Hydra visibility logic
  4. add logic to HydraNorth::Ability#custom_permissions to give a user cancan access rights to the show page of an object with an institutional visibility restriction iff the user is defined as belonging to that institution (which, for us, means they're CCID authenticated)
  5. modify the UI in various places (permissions edit, etc) to not label purely requiring authentication as being UofA specific, and to add the institutionally specific visibility option for the UofA.

Per the earlier conversation, CCID-restricted objects will still appear on index pages. Only the show page will require CCID authentication to view.

This should set us up fairly generically for extending repository service to other institutions, with only bits of the view that are UofA specific, and the auth logic that currently lives in the User model needing to be re-factored.

Based on the above plan I think I could have this done sometime Thursday, so it's probably 1-2 days easier than my earlier guess. Resizing to medium.

— Reply to this email directly or view it on GitHub < https://github.com/ualbertalib/HydraNorth/issues/582#issuecomment-140558681

.

— Reply to this email directly or view it on GitHub https://github.com/ualbertalib/HydraNorth/issues/582#issuecomment-140559668 .

Sharon Farnel Metadata Coordinator University of Alberta Libraries sharon.farnel@ualberta.ca 780-492-3685

pbinkley commented 9 years ago

Yes, this looks good.

The requirement to support other institutions is not likely to come up for some time (months/years), so we don't have to be thorough about removing hard-coded UofA specificity in the UI (step 5) for now; we'll have to review it all when the time comes anyway. So if anything there turns out to be a bog, leave it.

And re Sharon's point: could step 4 associate rights to the download operation rather than to the show page? That would mimic current ERA.

mbarnett commented 9 years ago

Sharon is correct.

Just took a quick look at it. Hydra/Sufia and more broadly CanCan apply visibility at a bit broader level (REST actions, basically), with Hydra assuming that you can download any resource you can 'read' (eg, any object you can visit the show page for).

We could split that apart with some clever monkeypatching of Hydra, potentially, but it's a tiny bit messier. Wouldn't necessarily take any longer, though.

pgwillia commented 8 years ago

This seems to have impacted collections. Besides the migration tests that seem to always fail, these tests are failing for me in this branch: Failures:

1) collection show collection as admin should allow me to nest collections Failure/Error: expect(page).to have_content("Is part of: #{community.title}") expected to find text "Is part of: Community" in "SyntaxError at /collections/6q182k15b syntax error, unexpected keyword_ensure, expecting keyword_end /var/www/sites/hydranorth/app/views/collections/_show_document_list_row.html.erb:70: syntax error, unexpected end-of-input, expecting keyword_end Application Frames All Frames app/views/collections/_show_document_list_row.html.erb, line 68 #Class:0x0000000dc80d68#_app_views_collectionsshow_document_list_html_erb2153838053162672898_99899980 app/views/collections/_show_document_list.html.erb, line 13 #Class:0x0000000dc80d68#_app_views_collections_show_html_erb___2613067697781603020_115730520 app/views/collections/show.html.erb, line 38 app/views/collections/_show_document_list_row.html.erb 63 64 65 66 Request info Request parameters {\"controller\"=>\"collections\", \"action\"=>\"show\", \"id\"=>\"6q182k15b\"} ...

./spec/features/collection/collection_spec.rb:65:in `block (3 levels) in <top (requir

ed)>'

2) collection delete items from collection should add and delete item from collection Failure/Error: expect(page).to have_content "Items in this Collection" expected to find text "Items in this Collection" in "SyntaxError at /collections/8623hx79n syntax error, unexpected keyword_ensure, expecting keyword_end /var/www/sites/hydranorth/app/views/collections/_show_document_list_row.html.erb:70: syntax error, unexpected end-of-input, expecting keyword_end Application Frames All Frames app/views/collections/_show_document_list_row.html.erb, line 68...

./spec/features/collection/collection_spec.rb:141:in `block (3 levels) in <top (required)>'

3) collection check collection for drop down menu options should not see edit and delete options Failure/Error: click_button("Select an action") Capybara::ElementNotFound: Unable to find button "Select an action"

./spec/features/collection/collection_spec.rb:168:in `block (3 levels) in <top (required)>'

Finished in 1 minute 53.93 seconds (files took 9.03 seconds to load) 12 examples, 3 failures

Failed examples:

rspec ./spec/features/collection/collection_spec.rb:56 # collection show collection as admin should allow me to nest collections rspec ./spec/features/collection/collection_spec.rb:137 # collection delete items from collection should add and delete item from collection rspec ./spec/features/collection/collection_spec.rb:167 # collection check collection for drop down menu options should not see edit and delete options

mbarnett commented 8 years ago

Looking into it

weiweishi commented 8 years ago

I've made changes to collections/community authorization in branch 92-communities_collections. I'm stuck with a controller spec test, and that has prevented to submit a PR. But things may be a little bit different in handling collections/communities.

Weiwei Shi

Digital Initiative Applications Librarian University of Alberta Libraries 2-10L Cameron Library Edmonton, Alberta, Canada T6G 2J8 Phone:(780)492-7802 Fax: (780)248-1209 Email: weiwei.shi@ualberta.ca

On Thu, Sep 24, 2015 at 11:25 AM, pgwillia notifications@github.com wrote:

This seems to have impacted collections. Besides the migration tests that seem to always fail, these tests are failing for me in this branch: Failures:

1) collection show collection as admin should allow me to nest collections Failure/Error: expect(page).to have_content("Is part of:

{community.title}")

expected to find text "Is part of: Community" in "SyntaxError at /collections/6q182k15b syntax error, unexpected keyword_ensure, expecting keyword_end /var/www/sites/hydranorth/app/views/collections/_show_document_list_row.html.erb:70: syntax error, unexpected end-of-input, expecting keyword_end Application Frames All Frames app/views/collections/_show_document_list_row.html.erb, line 68

Class:0x0000000dc80d68#_app_views_collectionsshow_document_list_html_erb2153838053162672898_99899980

app/views/collections/_show_document_list.html.erb, line 13

Class:0x0000000dc80d68#_app_views_collections_show_html_erb___2613067697781603020_115730520

app/views/collections/show.html.erb, line 38 app/views/collections/_show_document_list_row.html.erb 63 64 65 66 Request info Request parameters {\"controller\"=>\"collections\", \"action\"=>\"show\", \"id\"=>\"6q182k15b\"} ...

./spec/features/collection/collection_spec.rb:65:in `block (3 levels) in

ed)>'

2) collection delete items from collection should add and delete item from collection Failure/Error: expect(page).to have_content "Items in this Collection" expected to find text "Items in this Collection" in "SyntaxError at /collections/8623hx79n syntax error, unexpected keyword_ensure, expecting keyword_end /var/www/sites/hydranorth/app/views/collections/_show_document_list_row.html.erb:70: syntax error, unexpected end-of-input, expecting keyword_end Application Frames All Frames app/views/collections/_show_document_list_row.html.erb, line 68... ./spec/features/collection/collection_spec.rb:141:in `block (3 levels) in '

3) collection check collection for drop down menu options should not see edit and delete options Failure/Error: click_button("Select an action") Capybara::ElementNotFound: Unable to find button "Select an action"

./spec/features/collection/collection_spec.rb:168:in `block (3 levels)

in '

Finished in 1 minute 53.93 seconds (files took 9.03 seconds to load) 12 examples, 3 failures

Failed examples:

rspec ./spec/features/collection/collection_spec.rb:56 # collection show collection as admin should allow me to nest collections rspec ./spec/features/collection/collection_spec.rb:137 # collection delete items from collection should add and delete item from collection rspec ./spec/features/collection/collection_spec.rb:167 # collection check collection for drop down menu options should not see edit and delete options

— Reply to this email directly or view it on GitHub https://github.com/ualbertalib/HydraNorth/issues/582#issuecomment-142978866 .

mbarnett commented 8 years ago

It looks like it was a bad merge/bad copy-pasta on my part when I rebased for the PR. Pushing a fix in a sec.

mbarnett commented 8 years ago

Fixed and rolled the bug fixes on this branch back into a single clean commit

pgwillia commented 8 years ago

Thanks for that. Tests passing (including the migration ones?!).

There is one idiosyncrasy. Items with "Authenticated Access" image Show up as "University of Alberta Access" image

mbarnett commented 8 years ago

Good catch. I must have missed a spot, because Authenticated was being displayed as "" everywhere, previously. I'll add another fix for this.

Which brings up another issue -- we need to decide on an order for merging UX and this branch, and be careful not to blow away the UI separation of "Authenticated" and "" in this branch when we do the final merge

mbarnett commented 8 years ago

Fixed that issue. Turned out to be hardcoded inside of Sufia, so there's some monkeypatching happening in the Sufia initializer, now.