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

Wrapper fails to redirect on initial load #49

Closed
jcheroske opened this issue Jun 26, 2016 · 8 comments
Closed

Wrapper fails to redirect on initial load #49

jcheroske opened this issue Jun 26, 2016 · 8 comments
Labels

Comments

@jcheroske
Copy link

I believe this is a regression introduced in 0.5.1. It disappears completely when I roll back to 0.5.0.

If the authSelector returns null on the initial load of the page, the protected content will not load, but the configured redirect will fail to occur. If I login, the content will be correctly displayed. If I logout the correct redirect will take place.

@mjrussell
Copy link
Owner

@jcheroske hmm thats interesting...can you paste your auth wrapper config? (or even more of your code if you can) I did refactor the willMount logic slightly in 0.5.1 but I have a test for the the initial load that I would think cover this case you are describing.

@Windstalker
Copy link

Windstalker commented Jun 27, 2016

Reproduced this issue as well.

upd:
It seems, that if we are using router for redirection componentWillReceiveProps of auth-wrapped Component will not be called.

@mjrussell
Copy link
Owner

So the main change in 0.5.1 is to prevent infinite redirect loops which became more noticeable in react 15.1. The core of the issue is that previously to 0.5.1 the auth-wrapper library would call the redirect any time componentWillReceiveProps was called and the predicate returned false. This was pointed out to me as dangerous by the react team and that the redirection should only happen when authentication actually changes in cWRP not every time cWRP is called. So the login in cWRP became to only redirect if the predicate returned true for the previous props and returned false for the next props.

@jcheroske @Windstalker Are either of you using the isAuthenticating config parameter by any chance?

@Windstalker
Copy link

Windstalker commented Jun 27, 2016

@mjrussell Yes, I use it. Here is my config:

UserAuthWrapper({
  authSelector: (state) => {
    const { auth, user } = state;
    return { auth, user };
  },
  predicate: (authState) => {
    const { auth, user } = authState;
    return auth.accessToken.length && user.profile !== null;
  },
  authenticatingSelector: ({ session, user, auth }) => {
    const flag = !session.sessionReady || (auth.accessToken.length && user.profile === null);
    return flag;
  },
  redirectAction: routerActions.replace
});

OnApp.componentWillMount() I try to restore the session from local storage (access token is stored there). Using access token I try to fetch user profile from back-end API. Auth-wrapped component is the child component of the App.
So I have the next timeline on the clear launch of the app (not authorized):

  1. App.componentWillMount => reset session, authState: {isAuthenticating: true, isAuthorized: false}
AuthWrapped.componentWillMount() // Spinner rendered
  1. Session reset success => access token is empty, fetching user profile, authState: {isAuthenticating: true, isAuthorized: false}
AuthWrapped.componentWillReceiveProps() // Spinner rendered
  1. User profile fetch failure (401) => authState: {isAuthenticating: false, isAuthorized: false}
AuthWrapped.componentWillReceiveProps() // Empty rendered

isAuthorized for prev and next props are both false. So no redirection occurs. But expected behavior is redirection to login page

@mjrussell
Copy link
Owner

@Windstalker ok thanks this is perfect. I see the error that was introduced.

The componentWillReceiveProps needs to also check if isAuthenticating has changed from this.props and nextProps and trigger the redirect if isAuthorized is false.

I'll work on this today and get a new version out tonight that fixes this issue.

@Windstalker
Copy link

@mjrussell Would be great, thanks!

@jcheroske
Copy link
Author

You guys are on this! But, since I opened the dang thing, yes, I'm using the authenticatingSelector as well:

export const RequireAuthentication = UserAuthWrapper({
  authSelector: ({currentUser}) => currentUser,
  authenticatingSelector: ({auth}) => !auth.loginServicesReady || auth.loggingIn || auth.loggingOut,
  redirectAction: push,
  wrapperDisplayName: 'RequireAuthentication',
  allowRedirectBack: false
});

Thank you so much for the quick turnaround.

@mjrussell
Copy link
Owner

Fix released in 0.5.2. Thanks for the help on this

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

No branches or pull requests

3 participants