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 an exception for forgery token for external auth. #595

Merged
merged 1 commit into from Mar 8, 2017

Conversation

martinpovolny
Copy link

@martinpovolny martinpovolny commented Mar 6, 2017

should fix https://bugzilla.redhat.com/show_bug.cgi?id=1429011

basically we need to skip the csrf token check for all the methods that the login page can lead to

I hope I listed all, and I hope I have not put in any extra method

@jvlcek : please, test
ping @abellotti

what about about :saml_login and :initiate_saml_login ?

EUWE: should go to euwe together with #451

@miq-bot
Copy link
Member

miq-bot commented Mar 6, 2017

Checked commit martinpovolny@6b65095 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 1 offense detected

app/controllers/application_controller.rb

@imtayadeway
Copy link
Contributor

@martinpovolny what I don't understand about this bug is why excluding an action from this hook caused the issue, and why excluding more actions is the solution. Can you explain a little more in your description?

@martinpovolny
Copy link
Author

martinpovolny commented Mar 6, 2017

@imtayadeway : actually this line is causing the problem:

https://github.com/ManageIQ/manageiq-ui-classic/pull/451/files#diff-61341e9220760b0baf1ccde49150f8d1L13

Explanation: excluding the authenticate action resolved the original BZ. But the "housecleaning" -- removing the token from the login page -- broke the other actions that check for the token too.

There's no reason to check for the token in those pages. Actually they would exhibit the same bug as the "normal" login (when the session expires). So I am fixing this by excluding all the actions from the check.

@abellotti
Copy link
Member

Just finished testing with @jvlcek, all 👍 for external authentication (ldap, ipa), SSO (kerberos_authenticate), as well as ye-old SAML (keycloak).

Thanks @martinpovolny for this PR.

@mzazrivec mzazrivec added this to the Sprint 56 Ending Mar 13, 2017 milestone Mar 8, 2017
@mzazrivec mzazrivec merged commit 5fa64da into ManageIQ:master Mar 8, 2017
@martinpovolny martinpovolny deleted the csrf_skip branch March 8, 2017 08:42
@simaishi
Copy link
Contributor

simaishi commented Mar 9, 2017

Euwe backport (to manageiq repo) details:

$ git log -1
commit 337e83a28256a149aa8a61bec71cb2af2e57796f
Author: Milan Zázrivec <mzazrivec@redhat.com>
Date:   Wed Mar 8 09:42:12 2017 +0100

    Merge pull request #595 from martinpovolny/csrf_skip
    
    Adding an exception for forgery token for external auth.
    (cherry picked from commit 5fa64da998d2d758e289afb6a19d626ea3bc6f3d)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1430835

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

Successfully merging this pull request may close these issues.

None yet

7 participants