Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding existing works to existing collection fails #1191

Closed
awead opened this issue Jun 12, 2017 · 36 comments · Fixed by #2875
Closed

Adding existing works to existing collection fails #1191

awead opened this issue Jun 12, 2017 · 36 comments · Fixed by #2875
Assignees
Milestone

Comments

@awead
Copy link
Contributor

awead commented Jun 12, 2017

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

  1. Create a work and collection
  2. From the user dashboard, select the work and add it to the collection
@jcoyne
Copy link
Member

jcoyne commented Jun 12, 2017

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

@awead
Copy link
Contributor Author

awead commented Jun 12, 2017

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

@mjgiarlo mjgiarlo added this to the Backlog milestone Jun 12, 2017
@awead
Copy link
Contributor Author

awead commented Jun 12, 2017

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

@mjgiarlo
Copy link
Member

@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
Copy link
Member

(I ask because of samvera/hyku#980)

@awead
Copy link
Contributor Author

awead commented Jun 12, 2017

@mjgiarlo Hyrax test application generated in the hyrax repo.

@jcoyne
Copy link
Member

jcoyne commented Jun 13, 2017

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

@jcoyne
Copy link
Member

jcoyne commented Jun 15, 2017

@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
Copy link
Member

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:

<%= 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
Copy link
Member

jcoyne commented Jul 18, 2017

@mjgiarlo

its own new authenticity token

because button_to makes a new form with an authenticity token.

@mjgiarlo
Copy link
Member

mjgiarlo commented Jul 18, 2017

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
Copy link
Member

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
Copy link
Member

mjgiarlo commented Jul 18, 2017

@awead @elrayle @jcoyne @jrochkind ☝️

@jcoyne
Copy link
Member

jcoyne commented Jul 18, 2017

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

@mjgiarlo
Copy link
Member

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

@mjgiarlo
Copy link
Member

mjgiarlo commented Jul 18, 2017

@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
Copy link
Contributor

elrayle commented Jul 19, 2017

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

@atz
Copy link
Contributor

atz commented Aug 15, 2017

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
Copy link
Member

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

@atz
Copy link
Contributor

atz commented Aug 17, 2017

@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.

@jcoyne
Copy link
Member

jcoyne commented Sep 22, 2017

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
Copy link
Member

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
Copy link
Contributor

elrayle commented Oct 20, 2017

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
Copy link
Member

mjgiarlo commented Oct 20, 2017

@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
Copy link
Contributor

elrayle commented Nov 2, 2017

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

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
Copy link
Contributor

elrayle commented Nov 13, 2017

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

@jonathandixon
Copy link
Contributor

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
Copy link
Member

mjgiarlo commented Dec 21, 2017

@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
Copy link
Member

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

@jonathandixon
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member

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

@jonathandixon
Copy link
Contributor

@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.

elrayle added a commit that referenced this issue Jan 29, 2018
Related to issues
* #1191 - Adding existing works to existing collection fails
* #1489 - Throwing ActionController::InvalidAuthenticityToken when trying to add works to an existing collection
jgondron added a commit that referenced this issue Mar 29, 2018
- 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.
@jgondron jgondron self-assigned this Mar 29, 2018
@elrayle
Copy link
Contributor

elrayle commented Apr 1, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants