Bug #5105
closed
[Workbench] AJAX content loaders should not follow a redirect to the welcome page and use that as a partial, if token or session has been invalidated
Description
this happens when I have >2 tabs of curoverse open and then I log out in one of the tabs. I've seen this happen a couple times now. It may take a while to happen after logging out. I tried to reproduce it once but was unable to do so.
Files
Updated by Brett Smith almost 10 years ago
- Subject changed from Login page shows unncessary details after logging out to [Workbench] Login page sometimes shows ghost breadcrumbs, page tabs after logout
- Category set to Workbench
- Target version set to Bug Triage
Updated by Tom Clegg almost 10 years ago
Such overlay.
I am pretty 80% sure this means an auto-refresh AJAX request received [a redirect, which yielded] a welcome page. The AJAX handler dutifully inserted the HTML into a DIV somewhere... and this picture shows us how the browser renders <html><body>navbar...<div><html><body>navbar...</body></html></div></body></html>.
Updated by Tom Clegg almost 10 years ago
- Subject changed from [Workbench] Login page sometimes shows ghost breadcrumbs, page tabs after logout to [Workbench] AJAX content loaders should not follow a redirect to the welcome page and use that as a partial, if token or session has been invalidated
- Target version changed from Bug Triage to 2015-03-11 sprint
- Story points set to 1.0
Updated by Tom Clegg almost 10 years ago
- File 5105-tab-logged-out.png 5105-tab-logged-out.png added
Updated by Tom Clegg almost 10 years ago
Coincidentally, #5261 revealed a related bug, and fixed it with a general "don't respond to XHR with 302" change (57991d1 + 1c6d77f) which should correct most cases where "302-to-welcome" or "302-to-login" or "302-because-we-didn't-notice-this-was-AJAX" is silently accepted as "success" by client-side code.
Meanwhile, the 5105-ajax-redirect branch here (@ 797d78f) deals specifically with bugs where "302 can automatically fix the not-logged-in condition" logic was being attempted during AJAX requests, where it didn't fix anything, but did cause confusing results. The original bug report is represented in the "deleted session" test case: the new behavior puts an error message (instead of a "welcome to Arvados" page) in the tab's content area. A similar case (Workbench session is alive, but the API token is expired/invalid) is also fixed & tested.
One example turned up immediately: two of the "report issue" test cases were trying to submit an error report; getting a 302 response and following it to the welcome page (which of course responded 200); and concluding on the basis of the 200 that the error report had been sent successfully. This bug is fixed in d554f6e.
4e98958 + 797d78f + db1542b add some helpers for mocking API calls, and use them in a couple of basic unit tests for ArvadosApiClient. I didn't end up using mocks for the issue at hand, but they (and those little unit tests) seemed worth keeping anyway. Perhaps they can grow into a way to test a bunch of other Workbench units without invoking real API calls...
Updated by Tom Clegg almost 10 years ago
Just noticed that I still didn't fix the "report issue" feature enough to actually submit the user-entered text/info. It posts to report_issue, but it doesn't send any of the interesting data. Obviously, this still needs to be fixed & tested!
Updated by Tom Clegg almost 10 years ago
Tom Clegg wrote:
Just noticed that I still didn't fix the "report issue" feature enough to actually submit the user-entered text/info. It posts to report_issue, but it doesn't send any of the interesting data. Obviously, this still needs to be fixed & tested!
Updated by Brett Smith almost 10 years ago
Reviewing 6058d8a. The branch looks great, this is all pretty small potatoes.
- I wonder about the negative asserts in the new integration tests,
assert_no_selector '.container-fluid .container-fluid'andassert_no_text 'If you have never used'. These aren't very high-value assertions, just because it's so easy to make them pass by just tweaking little bits of wording or layout on the login page. Obviously a lot of that just comes with the territory of Selenium testing, but are the positive assertions in these tests sufficient for our purposes? - Should
ActiveSupport::TestCase.use_tokenrestore the old token in anensureblock? - This definitely doesn't have to happen in the branch, but I noticed it, so I'll share while it's on my mind: we might be getting to the point where it'd be worthwhile to decide how we want @errors to look. Full sentences or not? Should they strictly report ("Not logged in") or suggest ("Most likely your session has timed out and you need to log in again.")? Standard phrasing for the same error, style guide kind of stuff like that.
Thanks.
Updated by Tom Clegg almost 10 years ago
Brett Smith wrote:
Reviewing 6058d8a. The branch looks great, this is all pretty small potatoes.
- I wonder about the negative asserts in the new integration tests,
assert_no_selector '.container-fluid .container-fluid'andassert_no_text 'If you have never used'. These aren't very high-value assertions, just because it's so easy to make them pass by just tweaking little bits of wording or layout on the login page. Obviously a lot of that just comes with the territory of Selenium testing, but are the positive assertions in these tests sufficient for our purposes?
I wanted to check for "html html" or "body body" but the browser seems to automatically collapse those. I picked ".container-fluid .container-fluid" because it seemed like a relatively secure bootstrap thing: a responsive layout needs exactly one .container-fluid with everything in it.
"If you have never used" does seem likely to go away by itself and make the test weaker. What's the right pattern for this -- add a test for the sole purpose of asserting that the welcome page has the right identifying marks?
The positive assertions for "Reload tab" and "You are not logged in" focus on giving feedback to the user. I've added a ".pane-error-display" assertion, which seems like a better way to confirm that we're looking at a properly handled tab-loading error, not just some content that got rendered in the tab...
- Should
ActiveSupport::TestCase.use_tokenrestore the old token in anensureblock?
Yes, fixed. fed9582
- This definitely doesn't have to happen in the branch, but I noticed it, so I'll share while it's on my mind: we might be getting to the point where it'd be worthwhile to decide how we want @errors to look. Full sentences or not? Should they strictly report ("Not logged in") or suggest ("Most likely your session has timed out and you need to log in again.")? Standard phrasing for the same error, style guide kind of stuff like that.
Agreed. (Same goes for non-error text...)
Updated by Brett Smith almost 10 years ago
Tom Clegg wrote:
I wanted to check for
"html html"or"body body"but the browser seems to automatically collapse those. I picked".container-fluid .container-fluid"because it seemed like a relatively secure bootstrap thing: a responsive layout needs exactly one.container-fluidwith everything in it.
That sounds good, and I feel better about this assertion knowing it. A comment to that effect might help future readers understand too.
"If you have never used" does seem likely to go away by itself and make the test weaker. What's the right pattern for this -- add a test for the sole purpose of asserting that the welcome page has the right identifying marks?
I would rather assert against static page text as little as possible. It feels like we're making unnecessary busywork for ourselves if we have to run and change tests just to change some UI text for humans, and adding tests that just assert that exacerbate the problem.
The positive assertions for "Reload tab" and "You are not logged in" focus on giving feedback to the user. I've added a ".pane-error-display" assertion, which seems like a better way to confirm that we're looking at a properly handled tab-loading error, not just some content that got rendered in the tab...
I realize the primary intent of asserting the text "Reload tab" is to ensure the button is there, but it seems to also pull pretty effective double duty at asserting we didn't render the login page—what are the odds that it's going to say "Reload tab?" But if you still want a separate assertion that we didn't render the login page, what about asserting that there's no login link? That unit of the page is more functional, which makes it safer to assert against. And if you do it after a positive assertion (like assert_selector '.pane-error-display'), you don't have to worry about race conditions.
Thanks.
Updated by Tom Clegg almost 10 years ago
Brett Smith wrote:
That sounds good, and I feel better about this assertion knowing it. A comment to that effect might help future readers understand too.
Good point. Moved it to an assert_no_double_layout method, with a comment. 2d0dd74
"If you have never used" does seem likely to go away by itself and make the test weaker. What's the right pattern for this -- add a test for the sole purpose of asserting that the welcome page has the right identifying marks?
I would rather assert against static page text as little as possible. It feels like we're making unnecessary busywork for ourselves if we have to run and change tests just to change some UI text for humans, and adding tests that just assert that exacerbate the problem.
Agreed. I meant to include things like html pseudoclasses in "identifying marks". We can add special tags for testing that aren't entangled with UI text, but how do we prevent them from silently disappearing during layout-refactoring and making our assert_no_ tests ineffective?
I realize the primary intent of asserting the text "Reload tab" is to ensure the button is there, but it seems to also pull pretty effective double duty at asserting we didn't render the login page—what are the odds that it's going to say "Reload tab?" But if you still want a separate assertion that we didn't render the login page, what about asserting that there's no login link?
Although I didn't add one, I thought it would actually be quite reasonable to show a login link with the "you've been logged out" error message -- so asserting that we don't seemed a bit odd.
That unit of the page is more functional, which makes it safer to assert against. And if you do it after a positive assertion (like
assert_selector '.pane-error-display'), you don't have to worry about race conditions.
Fair enough, removed the "check for welcome message" assertions.
Updated by Tom Clegg almost 10 years ago
- Status changed from In Progress to Resolved