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

Check the request format before setting the redirection after the login page #10702

Conversation

@yaacov
Copy link
Member

yaacov commented Aug 23, 2016

Description
On rare events when an ajax request triggers the session timeout, and loads the
login page, the startup page will be a page not found error.

This PR prevent the page not found error in this cases. In the cases of an ajax request that triggers the timeout, we will not set the session[:start_url] to the url of the ajax call.

Sceenshot
screenshot-2016-08-23-19-11-56

Bugzilla
https://bugzilla.redhat.com/show_bug.cgi?id=1341664
https://bugzilla.redhat.com/show_bug.cgi?id=1370202

@yaacov

This comment has been minimized.

Copy link
Member Author

yaacov commented Aug 23, 2016

@miq-bot add_label providers/containers, bug

@simon3z @zeari @alongoldboim please review

@yaacov yaacov force-pushed the yaacov:prevent-ajax-calles-from-setting-the-startup-page branch Aug 23, 2016
@chessbyte chessbyte added the ui label Aug 23, 2016
@yaacov yaacov force-pushed the yaacov:prevent-ajax-calles-from-setting-the-startup-page branch 2 times, most recently Aug 24, 2016
@yaacov

This comment has been minimized.

Copy link
Member Author

yaacov commented Aug 24, 2016

A Note about the foreman specs
In spec/controllers/provider_foreman_controller_spec.rb we alter two tests.
This tests where false positive, they got response status 200 OK, but it was from the login page and not from a valid response for the test query.
After this PR any POST request without a valid credentials will get response status 403 Forbidden.

@yaacov yaacov closed this Aug 25, 2016
@yaacov yaacov reopened this Aug 25, 2016
@yaacov yaacov closed this Aug 25, 2016
@yaacov yaacov reopened this Aug 25, 2016
@yaacov yaacov closed this Aug 25, 2016
@yaacov yaacov reopened this Aug 25, 2016
@yaacov yaacov closed this Aug 28, 2016
@yaacov yaacov reopened this Aug 28, 2016
@simon3z

This comment has been minimized.

Copy link
Contributor

simon3z commented Aug 29, 2016

@yaacov why are we touching foreman specs?

@yaacov

This comment has been minimized.

Copy link
Member Author

yaacov commented Aug 29, 2016

@simon3z

why are we touching foreman specs?

The foreman specs are checking that the http response is 200-OK.
The request did not do what they intended to do, but they where successful because they got 200-OK from the login page ( they where redirected to it when authentication failed )

After this PR, ajax POST is no longer redirected to the login page, but return an "unauthorized" Error.
To fix the tests you have to login before running the ajax, now they actually work and return 200-OK.

@yaacov

This comment has been minimized.

Copy link
Member Author

yaacov commented Aug 30, 2016

@miq-bot miq-bot added the darga/yes label Aug 30, 2016
@moolitayer

This comment has been minimized.

Copy link
Contributor

moolitayer commented Aug 30, 2016

@yaacov this fix does not resolve the described issue. The unauthorized response causes the page to be stuck with the revolver and manually going to the login page still redirects to the same url.

@martinpovolny @himdel Could you please provide feedback on how to resolve this?

@yaacov yaacov force-pushed the yaacov:prevent-ajax-calles-from-setting-the-startup-page branch Aug 30, 2016
@yaacov

This comment has been minimized.

Copy link
Member Author

yaacov commented Aug 30, 2016

@moolitayer 👍 changed the logic of preventing an ajax request from setting session[:start_url]
moved from checking for POST , to setting session[:start_url] only for requests that has html format.

This make more sense since we want to have a .html page waiting for us after we login, we do not want to be sent to a .js page after we login.

@yaacov yaacov force-pushed the yaacov:prevent-ajax-calles-from-setting-the-startup-page branch Aug 30, 2016
@moolitayer

This comment has been minimized.

Copy link
Contributor

moolitayer commented Aug 30, 2016

@yaacov this version makes sense to me. What is the behavior now? (redirect? what happens after the login?)

@martinpovolny @himdel does it makes to not set start_url for ajax|js requests (curious, what are format.js requests?)

@yaacov

This comment has been minimized.

Copy link
Member Author

yaacov commented Aug 30, 2016

hat happens after the login?

@moolitayer when session[:start_url] is not set we are redirected to the default startup page. This is not a best solution, but it's beter than redirecting the user to a page not found.

@martinpovolny @himdel is their a way to know what was the last valid url ?

@moolitayer

This comment has been minimized.

Copy link
Contributor

moolitayer commented Aug 31, 2016

@martinpovolny could you please review

@yaacov yaacov changed the title Check the request method before redirecting to login page Check the request format before setting the redirection after the login page Sep 1, 2016
@moolitayer

This comment has been minimized.

Copy link
Contributor

moolitayer commented Sep 1, 2016

@dclarizio can we get a ui review please

@himdel

This comment has been minimized.

Copy link
Contributor

himdel commented Sep 1, 2016

I'm not sure this makes sense. There are URLs which are perfectly happy to respond both to format.js and to format.html - and rejecting them simply because an ajaxy transition was requested sounds wrong - I think what you really want to check for is whether it was a POST request or a GET request..

@himdel

This comment has been minimized.

Copy link
Contributor

himdel commented Sep 1, 2016

..Also, the reverse is true, format.html does not imply you can GET it..

@yaacov yaacov force-pushed the yaacov:prevent-ajax-calles-from-setting-the-startup-page branch 2 times, most recently Sep 1, 2016
On rare events when an ajax request trigers the session timout, and loads the
login page, the startup page will be a page not found error.
This PR prevent the page not found error in this cases. In the cases of an
ajax POST request that trigers the timeout, we will replay with an unautherized replay.

Bugzilla
https://bugzilla.redhat.com/show_bug.cgi?id=1341664
@yaacov yaacov force-pushed the yaacov:prevent-ajax-calles-from-setting-the-startup-page branch to 0022001 Sep 1, 2016
@miq-bot

This comment has been minimized.

Copy link
Member

miq-bot commented Sep 1, 2016

Checked commit yaacov@0022001 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
1 file checked, 0 offenses detected
Everything looks good. 🏆

@yaacov

This comment has been minimized.

Copy link
Member Author

yaacov commented Sep 1, 2016

@himdel 👍 thanks, changed the logic to GET vs POST

@himdel

This comment has been minimized.

Copy link
Contributor

himdel commented Sep 1, 2016

Thanks, I think this looks good, the code can cope just fine with the value being nil.. Should work :) 👍

@moolitayer

This comment has been minimized.

Copy link
Contributor

moolitayer commented Sep 1, 2016

LGTM 👍

@moolitayer

This comment has been minimized.

Copy link
Contributor

moolitayer commented Sep 1, 2016

Thanks @himdel

@martinpovolny martinpovolny merged commit d9e7b3e into ManageIQ:master Sep 1, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.0008%) to 31.565%
Details
@yaacov

This comment has been minimized.

Copy link
Member Author

yaacov commented Sep 1, 2016

This bug is darga/yes, needs back-porting:
https://bugzilla.redhat.com/show_bug.cgi?id=1370202

@simon3z

This comment has been minimized.

Copy link
Contributor

simon3z commented Sep 5, 2016

This bug is darga/yes, needs back-porting:
https://bugzilla.redhat.com/show_bug.cgi?id=1370202

cc @chessbyte

chessbyte added a commit that referenced this pull request Sep 6, 2016
…g-the-startup-page

Check the request format before setting the redirection after the login page
(cherry picked from commit d9e7b3e)
@chessbyte

This comment has been minimized.

Copy link
Member

chessbyte commented Sep 6, 2016

Darga Backport details:

$ git log

commit 71e564dd576596390c3e4dbb2f9150707f02cc51
Author: Martin Povolny <mpovolny@redhat.com>
Date:   Thu Sep 1 15:59:30 2016 +0200

    Merge pull request #10702 from yaacov/prevent-ajax-calles-from-setting-the-startup-page

    Check the request format before setting the redirection after the login page
    (cherry picked from commit d9e7b3e21723687dd12be96084875fdc75e32e88)
@chessbyte chessbyte added darga/backported and removed darga/yes labels Sep 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.