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

Fixing session validation propagation. #2498

Merged
merged 1 commit into from Jul 20, 2016

Conversation

Projects
None yet
2 participants
@dennisoelkers
Member

dennisoelkers commented Jul 19, 2016

When a store is initialising before the SessionStore is finished validating an existing session, it is listening on SessionActions.login.completed. Unfortunately this was not working in certain cases when there was a race between the promise validating the session and the initial call to SessionStore.isLoggedIn() in FetchProvider.json().

This is fixed by explicitly triggering the loginCompleted action, which is also taking over the state propagation in the store.

@dennisoelkers dennisoelkers added the bug label Jul 19, 2016

@dennisoelkers dennisoelkers added this to the 2.1.0 milestone Jul 19, 2016

@edmundoa edmundoa self-assigned this Jul 19, 2016

@edmundoa

This comment has been minimized.

Member

edmundoa commented Jul 19, 2016

It took me longer than expected to get to review this, and the authentication PR got merged while I was testing it.

I tried to rebase the changes, but the authentication PR also changed some behaviour in there, and I'm not sure if my rebase is doing what this PR is trying to fix (I'm not sure how to trigger the original issue tbh). Could you please take a look, @dennisoelkers? Maybe @kroepke can also help :)

Here's my patch, in case it helps. I tried to log-in and out with it and seems to work, but didn't use any fancy authentication things.

diff --git a/graylog2-web-interface/src/stores/sessions/SessionStore.js b/graylog2-web-interface/src/stores/sessions/SessionStore.js
index ef4d359..dabcce4 100644
--- a/graylog2-web-interface/src/stores/sessions/SessionStore.js
+++ b/graylog2-web-interface/src/stores/sessions/SessionStore.js
@@ -49,10 +49,7 @@ const SessionStore = Reflux.createStore({
     const username = Store.get('username');
     this._validateSession(sessionId).then((response) => {
       if (response.is_valid) {
-        this.loginCompleted({
-          sessionId: sessionId || response.session_id,
-          username: username || response.username,
-        });
+        SessionActions.login.completed({ sessionId: sessionId || response.session_id, username: username || response.username });
       } else {
         this._removeSession();
       }

Thank you!

Fixing session validation propagation.
When a store is initialising before the `SessionStore` is finished
validating an existing session, it is listening on
`SessionActions.login.completed`. Unfortunately this was not working on
certain cases when there was a race between the promise validating the
session and the initial call to `SessionStore.isLoggedIn()` in
`FetchProvider.json()`. This is fixed by explicitly triggering the
`loginCompleted` action, which is also taking over the state propagation
in the store.

@dennisoelkers dennisoelkers force-pushed the fix-session-propagation branch from 4245ad1 to 28760a2 Jul 20, 2016

@edmundoa

This comment has been minimized.

Member

edmundoa commented Jul 20, 2016

LGTM 👍

@edmundoa edmundoa merged commit 9802a85 into master Jul 20, 2016

4 checks passed

ci-server-integration Jenkins build graylog2-server-integration-pr 1126 has succeeded
Details
ci-web-linter Jenkins build graylog-pr-linter-check 612 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@edmundoa edmundoa deleted the fix-session-propagation branch Jul 20, 2016

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