Guards with StoreRouterConnectingModule

[csutorasr]

Bug, feature request, or proposal:

BUG

What is the expected behavior?

Not to loop forever.

What is the current behavior?

If I use a guard for authenticated routes with StoreRouterConnectingModule it gets stuck.

My code for easier understanding:

  canActivate(next: ActivatedRouteSnapshot, state: RouterStateSnapshot): boolean {
    const url: string = state.url;
    return this.checkLogin(url);
  }

  canActivateChild(childRoute: ActivatedRouteSnapshot, state: RouterStateSnapshot): boolean {
    return this.canActivate(childRoute, state);
  }

  checkLogin(url: string): boolean {
    if (this.authService.isLoggedIn) { return true; }
    this.authService.redirectURL = url;
    this.router.navigateByUrl('/login');
    return false;
  }

This guard checks if the users is logged in. The this.authService.isLoggedIn gets data from the state of the store. The this.authService.redirectURL dispatches an event to set the redirectURL in the store. Than the redirects to /login which is not have this guard. The routing from the router action (maybe changing state during another state change) causes the app stuck.

If I remove the StoreRouterConnectingModule from the app module everything works as guards and navigate and not handled by the ngrx.

What are the steps to reproduce?

Redirect from a guard with activated StoreRouterConnectingModule. I made a repo for the bug.

Which versions of Angular, NGRX, OS, TypeScript, CLI, browsers are affected?

Newest nightly with angular 4.2.6.

[Kaffiend]

@csutorasr Just experienced the same thing but finally realized that it obscures angular errors thrown for no providers for guards. The error is because the router store is not serializable due to _routerState being circular so when thrown it errors on the stringify of the _routerState. Just something to check..

[csutorasr]

@Kaffiend I have provided the guard. Do you have any idea what should I check?

[Kaffiend]

I've started removing the router from store and the connecting module to make sure there is no other underlying issue being obscured. If you remove these does it throw anything else or work as intended? In the pre 4.x.x version there was a go method we could call on the router store to handle routing within. I dont see that in 4.x

[csutorasr]

I tried go but I couldn't found either. I removed the StoreRouterConnectingModule then everything works. The problem should be something circular recall, as the browser always leaks memory.

[Kaffiend]

Personally i think we should be able to dispatch actions to the store for the router reducer to navigate within. But i havent dug that far into the router yet. Im still playing with feature modules and async store state.

[Kaffiend]

You could try adding an effect with the effects module for the router error actions dispatched by the router-store and see what you can catch there??

ROUTER_CANCEL and ROUTER_ERROR contain the store state before the navigation. Use the previous state to restore the consistency of the store.
from the documentation it looks like you can put an effect on these and handle the routing with an effect instead of within the guard itself. When the guard is not true, you should see the ROUTER_CANCEL action dispatched and react accordingly

[csutorasr]

There is no errors shown.

My problem is there aren't any errors at all. If an error showed up it would stop the loop between the states.

If it is easier to understand: imagine two effects that are calling each other. (1 => 2, 2=>1, 1=>2 ... ) The loop never end, but you got no errors and no stack-overflow as of observables. This is an easy problem to solve: use DAG effects.

The problem is the guard and the store does the same, but in a more hidden way. (1=> guard =>1, 1=> guard => 1)

I think a solution to this problem can be the use of effects in the router-store. The guards should be called by the handler of the effect not by angular-router.

[Kaffiend]

I havent done much with it yet, but I'm inclined to agree that effects are the way to go here. I'll be implement some similar things this evening myself so if i run across anything ill be sure to bring it up.

[Kaffiend]

looking at your updated code above, you could also create a custom selector for the router state and use that in your evaluation. The fact that your routing to guarded routes, and your not using the router store to handle it is probably why your getting the loop. Guards can return a Observable<boolean> | Promise<boolean> | boolean

export const selectUser = createFeatureSelector<fromLogin.LoginState>('user');
export const selectUserLoggedIn = createSelector(selectUser, (state: fromLogin.LoginState) => state.loggedIn);
export const selectRouter = createFeatureSelector<fromRouter.RouterReducerState>('router');
export const selectRouterUrl = createSelector(selectRouter, (state: fromRouter.RouterReducerState) => state.state.url);

@Injectable()
export class AuthGuard implements CanActivate {
    constructor(
        private router: Router,
        private store: Store<fromRoot.State>) {
    }
    public canActivate(
        route: ActivatedRouteSnapshot,
        state: RouterStateSnapshot,
    ): Observable<boolean> | Promise<boolean> | boolean {
        const routerState = this.store.select(fromRoot.selectRouterUrl);
        routerState.take(1).subscribe(url => console.log(url));
        const observable = this.store.select(fromRoot.selectUserLoggedIn);
        return observable;
    }
}

take will not leave an orphaned subscription.

[csutorasr]

I tried to do something you told me. If you look at my first code, there this.authService.isLoggedIn fetched the data from the service, I passed the observable instead of the value. The error is the same.

My code:

Guard:

  checkLogin(url: string): Observable<boolean> {
    this.authService.redirectURL = url;
    return this.authService.googleAuthenticated$;
  }

authService:

@Injectable()
export class AuthenticationService {
  googleAuthenticated$: Store<boolean>;
// ...

  constructor(private store: Store<AppStore>) {
    this.googleAuthenticated$ = this.store.select(state => state.core.authentication.googleAuthenticated);
// ...
  }
// ...
}

Probably the observable is fetched instantly (sync) and behaves as a boolean value.

[Kaffiend]

You will probably still need the effects, or you can try creating a slice of state for ui and dispatch to that reducer from the guard to handle the routing. I have the same need so when i figure it out ill let you know.

[Kaffiend]

This seems to work without any effects.
I added this meta reducer to the root reducer to see the navigation actions being dispatched and all looks normal. I can see the ROUTER_CANCEL actions on false guard checks and i could add effects to those as well. If you include the routerState its important you dont try to JSON.stringify it as it has circular props.

function logger(reducer: ActionReducer<State>) {
  return function(state: State, action: any) {
    console.log(`-------------${new Date().toLocaleTimeString()}---------------`);
    console.log(`State:`, state);
    console.log(`Action:`, action);
    console.log(`-------------END STATE LOGGER----------`);
    return reducer(state, action);
  };
}

export const developmentReducerFactory: ActionReducerFactory<State, Action> = compose(logger, combineReducers);

root reducer
this just selects that slice of state then drills down to the piece of that state i want.

export const selectUser = createFeatureSelector<fromLogin.LoginState>('user');
export const selectUserLoggedIn = createSelector(selectUser, (state: fromLogin.LoginState) => state.loggedIn);

auth.guard.ts
important thing to note here is i have the value set in state before I call for any routing.

@Injectable()
export class AuthGuard implements CanActivate {
    constructor(
        private router: Router,
        private store: Store<fromRoot.State>) {
    }
    public canActivate(
        route: ActivatedRouteSnapshot,
        state: RouterStateSnapshot,
    ): Observable<boolean> | Promise<boolean> | boolean {
        const observable: Observable<boolean> = this.store.select(fromRoot.selectUserLoggedIn);
        return observable.map(val => {
            if (val) {
                return true;
            } else {
                this.router.navigateByUrl('/');
                return false;
            }
        });
    }
}

is this in the same guard?

 canActivate(next: ActivatedRouteSnapshot, state: RouterStateSnapshot): boolean {
    const url: string = state.url;
    return this.checkLogin(url);
  }

  canActivateChild(childRoute: ActivatedRouteSnapshot, state: RouterStateSnapshot): boolean {
    return this.canActivate(childRoute, state);
  }

@csutorasr This is probably where your loop is coming from. How are you providing these guards functions in the module?

[csutorasr]

Have you tried to load a guarded page with the selectUserLoggedIn is false? For me that is the part which is not working. If it is true the navigation won't happen and the code works as theres no "circular" routing.

In my case /home is a guarded place. Coming from /login it works, loads everything as the user is logged in. But when refreshing/loading/browsing the http://localhost:4200/home is generating the memory leak.

I tried with this code which is very similar to yours.

checkLogin(url: string): Observable<boolean> {
    return this.store.select(s => s.core.authentication)
      .map(s => s.googleAuthenticated || s.facebookAuthenticated)
      .map(isLoggedIn => {
        if (isLoggedIn) {
          return true;
        }
        this.router.navigateByUrl('/');
        // this.authService.redirectURL = url; <= i'd like to have this line of code, but first it is nice without it.
        return false;
      })
  }

[Kaffiend]

can you push up a repo to github so i can look at it?

[csutorasr]

I made a repo for the error.

I hope it will help you. Tests won't run as I didn't care about them for this example.

[csutorasr]

I updated the repo as you said. Nothing have changed.

Pull and try that code as I wrote in the description.

[Kaffiend]

You need to add the operator.

…

On Jul 12, 2017 11:02 AM, "Robin Csutorás" ***@***.***> wrote: I updated the repo as you said. Nothing have changed. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#68 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHdrNRGuNPaCrnnT3r6VSfBEOFRObnRlks5sNOAAgaJpZM4OUIxf> .

[csutorasr]

I added the operator, it have pushed it to the master, still not working. Have you tried the code? I have no experiences with pull requests, but you can make one if you would like to. (I hope I will be able to accept it)

[Kaffiend]

i've tried and i am getting the same result, but this one is beyond me..i've checked and tried everything i can think of.

[csutorasr]

For the other problem (router redirection to the same page after login) I have a solution with an effect, where SetRedirectURLAction is setting the redirect url after login.

@Effect() routeCancelled$: Observable<Action> = this.actions$.ofType(ROUTER_CANCEL)
        .map(toPayload)
        .map(payload => new SetRedirectURLAction(payload.event.url));

Each time a guard cannot resolve the link, the url is saved.

[chan-dev]

any updates on this one? i'm using the current version and it has issues with guards as well