Adding existing works to existing collection fails #1191

awead · 2017-06-12T14:23:25Z

Descriptive summary

Given I have a work owned by me
And I have a collection owned by me
When I visit my dashboard
And select my work
And click "Add to Collection"
And select my collection
And click "Update Collection"
Then I should not see ActionController::InvalidAuthenticityToken

Expected behavior

My work should be added to my collection

Actual behavior

Started PUT "/collections/g158bh28p?locale=en" for 127.0.0.1 at 2017-06-12 10:21:54 -0400
Processing by Hyrax::CollectionsController#update as HTML
  Parameters: {"authenticity_token"=>"MdqIljoqgvEyFCRiXnKmGPbxMu8y7ykUlMWUamXLLqMkoLky/vAbpSiLprweauguBwQVCIeAYKTCEYrFzmPVLg==", "batch_document_ids"=>["5m60qr88b"], "collection"=>{"members"=>"add"}, "locale"=>"en", "id"=>"g158bh28p"}
Can't verify CSRF token authenticity.
Completed 422 Unprocessable Entity in 1ms (ActiveRecord: 0.0ms)



ActionController::InvalidAuthenticityToken - ActionController::InvalidAuthenticityToken:
  actionpack (5.1.1) lib/action_controller/metal/request_forgery_protection.rb:195:in `handle_unverified_request'
  actionpack (5.1.1) lib/action_controller/metal/request_forgery_protection.rb:227:in `handle_unverified_request'
  devise (4.3.0) lib/devise/controllers/helpers.rb:253:in `handle_unverified_request'
  actionpack (5.1.1) lib/action_controller/metal/request_forgery_protection.rb:222:in `verify_authenticity_token'
  activesupport (5.1.1) lib/active_support/callbacks.rb:413:in `block in make_lambda'
  activesupport (5.1.1) lib/active_support/callbacks.rb:197:in `block (2 levels) in halting'
  actionpack (5.1.1) lib/abstract_controller/callbacks.rb:12:in `block (2 levels) in <module:Callbacks>'
  activesupport (5.1.1) lib/active_support/callbacks.rb:198:in `block in halting'
  activesupport (5.1.1) lib/active_support/callbacks.rb:507:in `block in invoke_before'
  activesupport (5.1.1) lib/active_support/callbacks.rb:507:in `invoke_before'
  activesupport (5.1.1) lib/active_support/callbacks.rb:130:in `run_callbacks'
  actionpack (5.1.1) lib/abstract_controller/callbacks.rb:19:in `process_action'
  actionpack (5.1.1) lib/action_controller/metal/rescue.rb:20:in `process_action'
  actionpack (5.1.1) lib/action_controller/metal/instrumentation.rb:32:in `block in process_action'
  activesupport (5.1.1) lib/active_support/notifications.rb:166:in `block in instrument'
  activesupport (5.1.1) lib/active_support/notifications/instrumenter.rb:21:in `instrument'
  activesupport (5.1.1) lib/active_support/notifications.rb:166:in `instrument'
  actionpack (5.1.1) lib/action_controller/metal/instrumentation.rb:30:in `process_action'
  actionpack (5.1.1) lib/action_controller/metal/params_wrapper.rb:252:in `process_action'
  activerecord (5.1.1) lib/active_record/railties/controller_runtime.rb:22:in `process_action'
  actionpack (5.1.1) lib/abstract_controller/base.rb:124:in `process'
  actionview (5.1.1) lib/action_view/rendering.rb:30:in `process'
  actionpack (5.1.1) lib/action_controller/metal.rb:189:in `dispatch'
  actionpack (5.1.1) lib/action_controller/metal.rb:253:in `dispatch'
  actionpack (5.1.1) lib/action_dispatch/routing/route_set.rb:49:in `dispatch'
  actionpack (5.1.1) lib/action_dispatch/routing/route_set.rb:31:in `serve'
  actionpack (5.1.1) lib/action_dispatch/journey/router.rb:46:in `block in serve'
  actionpack (5.1.1) lib/action_dispatch/journey/router.rb:33:in `serve'
  actionpack (5.1.1) lib/action_dispatch/routing/route_set.rb:832:in `call'
  railties (5.1.1) lib/rails/engine.rb:522:in `call'
  railties (5.1.1) lib/rails/railtie.rb:185:in `method_missing'
  actionpack (5.1.1) lib/action_dispatch/routing/mapper.rb:17:in `block in <class:Constraints>'
  actionpack (5.1.1) lib/action_dispatch/routing/mapper.rb:46:in `serve'
  actionpack (5.1.1) lib/action_dispatch/journey/router.rb:46:in `block in serve'
  actionpack (5.1.1) lib/action_dispatch/journey/router.rb:33:in `serve'
  actionpack (5.1.1) lib/action_dispatch/routing/route_set.rb:832:in `call'
  warden (1.2.7) lib/warden/manager.rb:36:in `block in call'
  warden (1.2.7) lib/warden/manager.rb:35:in `call'
  rack (2.0.3) lib/rack/etag.rb:25:in `call'
  rack (2.0.3) lib/rack/conditional_get.rb:38:in `call'
  rack (2.0.3) lib/rack/head.rb:12:in `call'
  rack (2.0.3) lib/rack/session/abstract/id.rb:232:in `context'
  rack (2.0.3) lib/rack/session/abstract/id.rb:226:in `call'
  actionpack (5.1.1) lib/action_dispatch/middleware/cookies.rb:613:in `call'
  active-fedora (11.2.0) lib/active_fedora/ldp_cache.rb:26:in `call'
  flipflop (2.3.1) lib/flipflop/feature_cache.rb:12:in `call'
  activerecord (5.1.1) lib/active_record/migration.rb:556:in `call'
  actionpack (5.1.1) lib/action_dispatch/middleware/callbacks.rb:26:in `block in call'
  activesupport (5.1.1) lib/active_support/callbacks.rb:97:in `run_callbacks'
  actionpack (5.1.1) lib/action_dispatch/middleware/callbacks.rb:24:in `call'
  actionpack (5.1.1) lib/action_dispatch/middleware/executor.rb:12:in `call'
  better_errors (2.1.1) lib/better_errors/middleware.rb:84:in `protected_app_call'
  better_errors (2.1.1) lib/better_errors/middleware.rb:79:in `better_errors_call'
  better_errors (2.1.1) lib/better_errors/middleware.rb:57:in `call'
  actionpack (5.1.1) lib/action_dispatch/middleware/debug_exceptions.rb:59:in `call'
  actionpack (5.1.1) lib/action_dispatch/middleware/show_exceptions.rb:31:in `call'
  railties (5.1.1) lib/rails/rack/logger.rb:36:in `call_app'
  railties (5.1.1) lib/rails/rack/logger.rb:24:in `block in call'
  activesupport (5.1.1) lib/active_support/tagged_logging.rb:69:in `block in tagged'
  activesupport (5.1.1) lib/active_support/tagged_logging.rb:26:in `tagged'
  activesupport (5.1.1) lib/active_support/tagged_logging.rb:69:in `tagged'
  railties (5.1.1) lib/rails/rack/logger.rb:24:in `call'
  sprockets-rails (3.2.0) lib/sprockets/rails/quiet_assets.rb:13:in `call'
  actionpack (5.1.1) lib/action_dispatch/middleware/remote_ip.rb:79:in `call'
  actionpack (5.1.1) lib/action_dispatch/middleware/request_id.rb:25:in `call'
  rack (2.0.3) lib/rack/method_override.rb:22:in `call'
  rack (2.0.3) lib/rack/runtime.rb:22:in `call'
  activesupport (5.1.1) lib/active_support/cache/strategy/local_cache_middleware.rb:27:in `call'
  actionpack (5.1.1) lib/action_dispatch/middleware/executor.rb:12:in `call'
  actionpack (5.1.1) lib/action_dispatch/middleware/static.rb:125:in `call'
  rack (2.0.3) lib/rack/sendfile.rb:111:in `call'
  railties (5.1.1) lib/rails/engine.rb:522:in `call'
  puma (3.9.1) lib/puma/configuration.rb:224:in `call'
  puma (3.9.1) lib/puma/server.rb:602:in `handle_request'
  puma (3.9.1) lib/puma/server.rb:435:in `process_client'
  puma (3.9.1) lib/puma/server.rb:299:in `block in run'
  puma (3.9.1) lib/puma/thread_pool.rb:120:in `block in spawn_thread'

Steps to reproduce the behavior

Create a work and collection
From the user dashboard, select the work and add it to the collection

The text was updated successfully, but these errors were encountered:

jcoyne · 2017-06-12T14:49:50Z

I was unable to duplicate this error. What version of Hyrax are you testing?

awead · 2017-06-12T14:58:41Z

@jcoyne latest on master as of last week. Let me try re-generating and see what happens.

awead · 2017-06-12T15:38:21Z

@jcoyne no, I'm still getting it. I've regenerated the application, and used two different browsers.

mjgiarlo · 2017-06-12T16:16:46Z

@awead are you seeing this with the test app? or with scholarsphere/another app? If the latter, can you share the contents of config/initializers/hyrax.rb?

mjgiarlo · 2017-06-12T16:16:59Z

(I ask because of samvera/hyku#980)

awead · 2017-06-12T16:48:47Z

@mjgiarlo Hyrax test application generated in the hyrax repo.

jcoyne · 2017-06-13T21:01:12Z

I'm getting this too, but only in development mode.

jcoyne · 2017-06-15T04:42:28Z

@awead could this be a turbolinks bug? This is an interesting case where it's trying to submit an authenticity token for a button_to form, that is different from the authenticity token in the header <meta> tag.

Are we tracking rails/rails#24257 ?

mjgiarlo · 2017-07-18T20:53:17Z

I did a bunch more testing on this today and confirmed the bug in RAILS_ENV=production.

The button next to the Add Collection button works. Here are the two buttons:

hyrax/app/views/hyrax/collections/_form_for_select_collection.html.erb

Lines 34 to 35 in e2bf67b

    
                       <%= render 'hyrax/collections/button_for_update_collection', label: t("hyrax.collection.select_form.update"), collection_id: 'collection_replace_id' %> 
        
                       <%= render 'hyrax/collections/button_create_collection', label: t("hyrax.collection.select_form.create_new") %>

Only the button on line 34 renders with its own new authenticity token, possibly because of method: :put?

When I 1) disable Turbolinks, and 2) change :put to :patch for that button, it works as expected. I am not sure why. (Meanwhile, this also works fine in Hyku, which uses the same code here and has turbolinks turned on.)

jcoyne · 2017-07-18T21:00:27Z

@mjgiarlo

its own new authenticity token

because button_to makes a new form with an authenticity token.

mjgiarlo · 2017-07-18T21:22:54Z

OK, after more testing, I got some stuff wrong earlier.

First, turbolinks doesn't seem to be related.

As a test, I'm rendering the button twice now, once with method: :put and once with method: :patch. And the :put vs. :patch is misleading. Here's what happens: whichever button I click first (whether put or patch) fails. Whichever one I click second works, even if it's the same one. @jcoyne earlier said that a reload might fix the behavior, and while I couldn't get a reload to do the trick, submitting the form once then back-buttoning seems to make things work (which makes it sound turbolinks-related? ugh.).

It's not the case that :patch is OK and :put is not like I suggested earlier.

mjgiarlo · 2017-07-18T22:04:13Z

The method I was using to remove turbolinks was incomplete (removing it from application.js and then recompiling assets). When I remove it from the application's Gemfile and bundle (also removing it from application.js), this error no longer happens.

Worth saying, again, that this is not an issue we've seen in Hyku despite it using the same version of Rails (5.1.2) and Turbolinks (5.0.1) that I've been testing with a vanilla Hyrax test app today.

mjgiarlo · 2017-07-18T22:12:37Z

@awead @elrayle @jcoyne @jrochkind ☝️

jcoyne · 2017-07-18T22:13:40Z

I wonder if it's due to the version of jquery that is used?

mjgiarlo · 2017-07-18T22:15:15Z

@jcoyne Ah, perhaps. Hyku uses jquery3. I'll test that out.

mjgiarlo · 2017-07-18T22:31:28Z

@jcoyne Changing //= require jquery to //= require jquery3 and then cleaning/clobbering/precompiling assets does not seem to account for the difference between Hyku and the test app, unfortunately.

elrayle · 2017-07-19T13:46:51Z

FYI... when I pinned rails to 5.0.3, the problem at least appears to have gone away.

atz · 2017-08-15T23:18:02Z

Unclear to me what the proposed fix is for Hyrax. Can somebody clarify? Do we want to bump the version of rails required? Version of jquery required? Do something w/ turbolinks?

mjgiarlo · 2017-08-15T23:44:52Z

@atz Unclear. Needs more analysis to figure out what the real culprit is. Removing turbolinks seems like a suboptimal solution to me.

atz · 2017-08-17T00:48:42Z

@mjgiarlo Since the problem seems to be limited to this page, we could add data-no-turbolink for just it. I'm not much in favor of figuring out the real cause as much as figuring out the real fix.

Refs #1191 Follows #1605

jcoyne · 2017-09-22T14:39:58Z

I wonder if only occurs when: Rails.application.config.action_controller.per_form_csrf_tokens = false (this is in config/initializers/new_framework_defaults.rb on rails 5). Has anyone checke this?

mjgiarlo · 2017-09-22T15:28:40Z

Thanks for the tip, @jcoyne! I don't believe anyone's checked this. If this is the case, we should ask folks how they'd feel about twiddling this value.

elrayle · 2017-10-20T19:43:37Z

Where are we on this issue? I'm asking because we still keep stumbling across this in the collections-sprint branch. We've added turbolinks: false in a number of places, but it would be good to have a general solution.

mjgiarlo · 2017-10-20T20:29:06Z

@elrayle Where we are:

@jcoyne's last comment may provide a clue as to what's causing the root behavior. Until someone looks into the prior point or finds another fix, we've been disabling turbolinks on inbound links. See these two PRs, which you will want to apply to other, similar links in the collections-sprint branch: https://github.com/samvera/hyrax/pull/1605/files and https://github.com/samvera/hyrax/pull/1614/files

elrayle · 2017-11-02T14:37:31Z

I took another run at this but having no luck.

I tried turning on/off each of these configs in all possible combinations. When the errors occur, I can confirm that value of these settings are as expected. But they seem to have no impact.

# Avoid ActionController::InvalidAuthenticityToken errors
Rails.application.config.action_controller.per_form_csrf_tokens = false
Rails.application.config.action_controller.forgery_protection_origin_check = false
# Rails.application.config.action_controller.allow_forgery_protection = false

From https://github.com/plataformatec/devise/pull/4033/files

For Rails 5, note that protect_from_forgery is no longer prepended to the before_action chain, so if you have set authenticate_user before protect_from_forgery, your request will result in "Can't verify CSRF token authenticity." To resolve this, either change the order in which you call them, or use protect_from_forgery prepend: true.

I tried updating my_controller.rb with several options set on protect_from_forgery...

    # protect_from_forgery               # forwards to Dashboard with msg: 'You are already signed in.'
    # protect_from_forgery prepend: true # forwards to Dashboard with msg: 'You are already signed in.'
    # protect_from_forgery prepend: true, with: :exception # gets the error
    protect_from_forgery with: :exception                  # gets the error.  NOTE:  This is the one most recommended.

    before_action :authenticate_user!

Presumably prepend: true is not needed as long as protect_from_forgery comes before the before_action :authenticate_user! statement. But I tested with and without it just in case.

I tried skipping...

    skip_before_action :verify_authenticity_token       # gets the error

Places I found interesting information...

elrayle · 2017-11-13T15:13:22Z

@jcoyne pointed to this github issue as relevant to this conversation...

Missing Authenticity Token in Remote Forms w/ File Upload Field(s) rails/rails#22807

jonathandixon · 2017-12-21T20:27:52Z

I may have figured out what's happening in the case of batch adding works to a collection. It relates to turbolinks and jquery-ujs.

It appears the default setting is to generate a CSRF token for each form, i.e. Rails.application.config.action_controller.per_form_csrf_tokens = true. This contradicts what we see when a page loads without turbolinks, the tokens in the forms and the global token in the meta tag match. As it turns out jquery-ujs executes $.rails.refreshCSRFTokens() on page loads which copies the token from the meta tag to the form inputs. When a page loads with turbolinks the JS that copies the token is not executed. So this how we end up with different tokens in the HTML.

Normally, this would not be a problem because all of the CSRF tokens that were loaded are valid. Here's the interesting part, in this particular case the token was generated for the action /dashboard/collections/collection_replace_id, but the actual action depends on the selected collection. Selecting a collection modifies the action path, thus invalidating the token. Using the global token (the one defined in the meta tag) works because it is not specific to the action.

With that being said, it seems like setting Rails.application.config.action_controller.per_form_csrf_tokens = false would solve the problem.

mjgiarlo · 2017-12-21T20:49:18Z

@jonathandixon Wow, great find! This looks like the same: rails/jquery-ujs#456

And see the workaround suggested here: rails/jquery-ujs#456 (comment)

mjgiarlo · 2017-12-21T20:54:04Z

@jonathandixon see also @elrayle's message above that suggests setting per_form_csrf_tokens to false does not seem to work.

jonathandixon · 2017-12-21T21:21:11Z

I can confirm that setting per_form_csrf_tokens = false does not force the same token for all forms on the page in question.

jonathandixon · 2017-12-21T22:02:07Z

My last comment is incorrect. per_form_csrf_tokens = false is honored but tokens are masked (https://github.com/rails/rails/blob/e88e6cea2113ce3e54410cbd8c2da92b86f83d2b/actionpack/lib/action_controller/metal/request_forgery_protection.rb#L307).

Which put's me back at square 1 in trying to figure this out.

mjgiarlo · 2017-12-21T22:03:19Z

@jonathandixon I haven't had time to dig in, but have you looked at this workaround? rails/jquery-ujs#456 (comment)

jonathandixon · 2017-12-22T14:26:35Z

@mjgiarlo I only tried running $.rails.refreshCSRFTokens() in the developer console, which does work. I have a feeling something like this would work

$(document).on('turbolinks:load', function() {
  $.rails.refreshCSRFTokens();
});

I will try to do some more testing after the holidays.

Related to issues * #1191 - Adding existing works to existing collection fails * #1489 - Throwing ActionController::InvalidAuthenticityToken when trying to add works to an existing collection

- Found the root issue is related to turbolinks + jquery-ujs + a button that uses ajax to post. This may also fix #1191. Adding a call to refresh the CSRF tokens in the rendered DOM after turbolinks loads fixes it.

elrayle · 2018-04-01T01:47:47Z

The scenario in this issue has been fixed for a while. And we now have a PR that seems to fix the general issue of turbolinks raising the invalid authenticity tokens exception. See PR #2875.

I think we can close this one when the PR is merged. Any objections? @awead @jcoyne @mjgiarlo @no-reply @jonathandixon @atz @martyn-w

mjgiarlo added this to the Backlog milestone Jun 12, 2017

mjgiarlo added the feedback needed label Jun 12, 2017

jcoyne added bug and removed feedback needed labels Jun 13, 2017

jcoyne marked this as a duplicate of #1362 Jul 18, 2017

jcoyne mentioned this issue Jul 18, 2017

ActionController::InvalidAuthenticityToken when adding a work to a collection #1362

Closed

mjgiarlo modified the milestones: 2.0.0, Backlog Jul 18, 2017

mjgiarlo added the backport needed label Jul 18, 2017

mjgiarlo mentioned this issue Aug 15, 2017

Throwing ActionController::InvalidAuthenticityToken when trying to add works to an existing collection #1489

Closed

mjgiarlo added a commit that referenced this issue Sep 1, 2017

Disable turbolinks on links to my_works_path

d1b80fc

Refs #1191 Follows #1605

mjgiarlo mentioned this issue Sep 1, 2017

Disable turbolinks on links to my_works_path #1614

Merged

vantuyls modified the milestones: 2.0.0, 2.x series Sep 20, 2017

jonathandixon mentioned this issue Jan 10, 2018

Add batch actions to all/managed works page. #2434

Merged

elrayle mentioned this issue Jan 29, 2018

Avoid ‘Can't verify CSRF token authenticity.’ when saving a collection #2571

Merged

elrayle mentioned this issue Feb 15, 2018

SWW error when adding Embargoed work to collection samvera-labs/nurax-pre2023#176

Closed

conorom mentioned this issue Feb 26, 2018

Clicking on asset produces 422 error mlibrary/heliotrope#1580

Closed

jgondron mentioned this issue Mar 29, 2018

Fix for #2873 - CSRF Exception after adding to Collections/Sharing #2875

Merged

jgondron self-assigned this Mar 29, 2018

jgondron added the in progress label Mar 29, 2018

elrayle mentioned this issue Mar 31, 2018

Test and remove all instances of data: { turbolinks: false } if no longer needed #2881

Closed

no-reply closed this as completed in #2875 Apr 2, 2018

no-reply removed the in progress label Apr 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding existing works to existing collection fails #1191

Adding existing works to existing collection fails #1191

awead commented Jun 12, 2017

jcoyne commented Jun 12, 2017

awead commented Jun 12, 2017

awead commented Jun 12, 2017

mjgiarlo commented Jun 12, 2017

mjgiarlo commented Jun 12, 2017

awead commented Jun 12, 2017

jcoyne commented Jun 13, 2017

jcoyne commented Jun 15, 2017 •

edited

Loading

mjgiarlo commented Jul 18, 2017

jcoyne commented Jul 18, 2017

mjgiarlo commented Jul 18, 2017 •

edited

Loading

mjgiarlo commented Jul 18, 2017

mjgiarlo commented Jul 18, 2017 •

edited

Loading

jcoyne commented Jul 18, 2017

mjgiarlo commented Jul 18, 2017

mjgiarlo commented Jul 18, 2017 •

edited

Loading

elrayle commented Jul 19, 2017

atz commented Aug 15, 2017

mjgiarlo commented Aug 15, 2017

atz commented Aug 17, 2017

jcoyne commented Sep 22, 2017

mjgiarlo commented Sep 22, 2017

elrayle commented Oct 20, 2017

mjgiarlo commented Oct 20, 2017 •

edited

Loading

elrayle commented Nov 2, 2017 •

edited

Loading

elrayle commented Nov 13, 2017

jonathandixon commented Dec 21, 2017

mjgiarlo commented Dec 21, 2017 •

edited

Loading

mjgiarlo commented Dec 21, 2017

jonathandixon commented Dec 21, 2017

jonathandixon commented Dec 21, 2017

mjgiarlo commented Dec 21, 2017

jonathandixon commented Dec 22, 2017

elrayle commented Apr 1, 2018

Adding existing works to existing collection fails #1191

Adding existing works to existing collection fails #1191

Comments

awead commented Jun 12, 2017

Descriptive summary

Expected behavior

Actual behavior

Steps to reproduce the behavior

jcoyne commented Jun 12, 2017

awead commented Jun 12, 2017

awead commented Jun 12, 2017

mjgiarlo commented Jun 12, 2017

mjgiarlo commented Jun 12, 2017

awead commented Jun 12, 2017

jcoyne commented Jun 13, 2017

jcoyne commented Jun 15, 2017 • edited Loading

mjgiarlo commented Jul 18, 2017

jcoyne commented Jul 18, 2017

mjgiarlo commented Jul 18, 2017 • edited Loading

mjgiarlo commented Jul 18, 2017

mjgiarlo commented Jul 18, 2017 • edited Loading

jcoyne commented Jul 18, 2017

mjgiarlo commented Jul 18, 2017

mjgiarlo commented Jul 18, 2017 • edited Loading

elrayle commented Jul 19, 2017

atz commented Aug 15, 2017

mjgiarlo commented Aug 15, 2017

atz commented Aug 17, 2017

jcoyne commented Sep 22, 2017

mjgiarlo commented Sep 22, 2017

elrayle commented Oct 20, 2017

mjgiarlo commented Oct 20, 2017 • edited Loading

elrayle commented Nov 2, 2017 • edited Loading

elrayle commented Nov 13, 2017

jonathandixon commented Dec 21, 2017

mjgiarlo commented Dec 21, 2017 • edited Loading

mjgiarlo commented Dec 21, 2017

jonathandixon commented Dec 21, 2017

jonathandixon commented Dec 21, 2017

mjgiarlo commented Dec 21, 2017

jonathandixon commented Dec 22, 2017

elrayle commented Apr 1, 2018

jcoyne commented Jun 15, 2017 •

edited

Loading

mjgiarlo commented Jul 18, 2017 •

edited

Loading

mjgiarlo commented Jul 18, 2017 •

edited

Loading

mjgiarlo commented Jul 18, 2017 •

edited

Loading

mjgiarlo commented Oct 20, 2017 •

edited

Loading

elrayle commented Nov 2, 2017 •

edited

Loading

mjgiarlo commented Dec 21, 2017 •

edited

Loading