Skip protect_from_forgery for #authenticate #451
Conversation
|
I'd also remove |
It makes no sense to check the token for the authenticate action. It only makes the product seem broken when you return to a login screen after the session has expired.
martinpovolny
force-pushed
the
skip_pff_for_authenticate
branch
from
ae7609e
to
0f99d9a
Compare
|
@himdel : done |
|
Verified in UI 👍 :
Before: broken |
|
Checked commit martinpovolny@0f99d9a with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
|
@martinpovolny turns out I had a BZ for this problem (https://bugzilla.redhat.com/show_bug.cgi?id=1417661) .. added to description, and adding |
|
Should have been mine! I could have claimed I worked on it the whole Monday while I would actually delete some code! |
|
Feel free to take it :D |
| @@ -16,7 +16,7 @@ class ApplicationController < ActionController::Base | |||
| # This secret is reset to a value found in the miq_databases table in | |||
| # MiqWebServerWorkerMixin.configure_secret_token for rails server, UI, and | |||
| # web service worker processes. | |||
| protect_from_forgery :secret => SecureRandom.hex(64), :except => :csp_report, :with => :exception | |||
| protect_from_forgery :secret => SecureRandom.hex(64), :except => [:authenticate, :csp_report], :with => :exception | |||
There was a problem hiding this comment.
/cc @jvlcek if this is related to the ext-auth issue you were troubleshooting, we might need to add exceptions for the other two methods in dashboard_controller, namely :external_authenticate (for ext-auth), and :kerberos_authenticate (SSO). Thanks.
There was a problem hiding this comment.
/cc @jvlcek we should be ok with SAML, but probably need to test that too. Thanks.
There was a problem hiding this comment.
@abellotti : what about :saml_login, :initiate_saml_login ?
|
EUWE: should go together with: #595 |
|
Euwe backport (to manageiq repo) details: |
In ManageIQ/manageiq-ui-classic#451 we removed the CSRF token in our login page, which breaks the requestor code when trying to log in and the csrf token is no where to be found. By using the block form, we can return the proper match in the CSRF token regexp if it exists, but still return `nil` if it does not, which will be fine it the header is blank (which is why a `.to_s` was added to the caller). Instead of removing the csrf token lookup on login, we are keeping this in place to allow this to be used on older versions of the MIQ application, otherwise we would need conditional logic for different versions of the application.
It makes no sense to check the token for the authenticate action.
It only makes the product seem broken when you return to a login screen
after the session has expired.
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1417661
EUWE BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1427172
@mzazrivec, @himdel: review?