Multiple canActivate guards all run when first fails
Asked Answered
H

6

76

I have a route with two canActivate guards (AuthGuard and RoleGuard). The first (AuthGuard) checks to see if the user is logged in and, if not, redirects to the login page. The second checks to see if the user has a role defined that is allowed to view the page and, if not, redirects to the un-authorized page.

canActivate: [ AuthGuard, RoleGuard ]
...
export class AuthGuard implements CanActivate {
    canActivate(route: ActivatedRouteSnapshot, state: RouterStateSnapshot): Promise<boolean> {
        ...
        this.router.navigate(['/login']);
        resolve(false);
}

export class RoleGuard implements CanActivate {
    canActivate(route: ActivatedRouteSnapshot, state: RouterStateSnapshot): Promise<boolean> {
        ...
        this.router.navigate(['/unauthorized']);
        resolve(false);
}

The problem is that when I access the route and I am not logged in I hit the AuthGuard, which fails and tells the router to navigate to /login. However, even though the AuthGuard failed, the RoleGuard runs anyway which then navigates to /unauthorized.

In my opinion it is pointless to run the next guard if the first fails. Is there any way to enforce this behavior?

Hear answered 14/11, 2016 at 13:27 Comment(5)
This is no longer an issue as from Angular 7.1 and above. Check my answer with a reference to a nice blog post on the topic hereProudfoot
Does this answer your question? Execute Multiple Asynchronous Route Guards in OrderTumultuous
@DzinX This question is earlier, has more upvotes and more answers. So that one should be a dupe of this, if anything. You shouldn't be biased by the fact that you answered that one. You can also improve the wording of this one if you think its an issue of question quality / reach.Madelynmademoiselle
@Madelynmademoiselle I saw all questions on this topic before I decided to answer, and decided to answer the other one since it was asked in a better, more direct way. Also ,this question's answers are mostly obsolete and incorrect as they talk about behavior of Angular that's not there anymore. That's why I think it's better to archive this question and not the other one. Correcting all answers in this thread seems like a worse idea to me.Tumultuous
@DzinX ok, gotcha. So you think this question is so outdated it no longer has value, and even the great answers are not irrelevant? It does have an "angular2-routing" tag. And wha't SO's policty / best practice on this? Is there a Meta post discussing it?Madelynmademoiselle
F
49

In the latest Angular version, even if both guards return false, both will still be executed.

You can however resolve this with your example by only using RoleGuard for urls where a certain Role is required, because I guess you need to be logged in to have a role. In that case you can change your RoleGuard to this:

@Injectable()
export class RoleGuard implements CanActivate {
  constructor(private _authGuard: AuthGuard) {}

  canActivate(route: ActivatedRouteSnapshot, state: RouterStateSnapshot): Promise<boolean> {
    return this._authGuard.canActivate(route, state).then((auth: boolean) => {
      if(!auth) {
        return false;
      }
      //... your role guard check code goes here
    });
  }
}
Frederickson answered 14/11, 2016 at 14:15 Comment(4)
Thank you for the suggestion PierreDuc. I realize combining the two guards would ultimately work in this specific scenario but I want to avoid this kind of solution because I have others scenarios where this would not work. If returning a boolean produces the expected behavior but returning a Promise<boolean> does not then I assume this is a router bug.Hear
Accepting your answer as the solution since it seems like the most logical workaround and I will post this as a bug on Angular git.Thanks again.Hear
@Hear thanks for your accept. i tried looking for your issue on git to upvote it, but couldn't find itFrederickson
@Pierre - thank's I'll update (I'll up vote once the edit will be accepted - It's currently locked)Cabernet
F
23

As mentioned by @PierreDuc data property in Route Class along with a Master Guard can be used to solve this problem.

Problem

First of all, angular doesn't support the feature to call the guards in tandem. So if first guard is asynchronous and is trying to make ajax calls, all the remaining guards will get fired even before completion of the ajax request in guard 1.

I faced the similar problem and this is how I solved it -


Solution

The idea is to create a master guard and let the master guard handle the execution of other guards.

The routing configuration in this case, will contain master guard as the only guard.

To let master guard know about the guards to be triggered for specific routes, add a data property in Route.

The data property is a key value pair that allows us to attach data with the routes.

The data can then be accessed in the guards using ActivatedRouteSnapshot parameter of canActivate method in the guard.

The solution looks complicated but it will assure proper working of guards once it is integrated in the application.

Following example explains this approach -


Example

1. Constants Object to map all application guards -

export const GUARDS = {
    GUARD1: "GUARD1",
    GUARD2: "GUARD2",
    GUARD3: "GUARD3",
    GUARD4: "GUARD4",
}

2. Application Guard -

import { Injectable } from "@angular/core";
import { Guard4DependencyService } from "./guard4dependency";

@Injectable()
export class Guard4 implements CanActivate {
    //A  guard with dependency
    constructor(private _Guard4DependencyService:  Guard4DependencyService) {}

    canActivate(next: ActivatedRouteSnapshot, state: RouterStateSnapshot): Promise<boolean> {
        return new Promise((resolve: Function, reject: Function) => {
            //logic of guard 4 here
            if (this._Guard4DependencyService.valid()) {
                resolve(true);
            } else {
                reject(false);
            }
        });
    }
}

3. Routing Configuration -

import { Route } from "@angular/router";
import { View1Component } from "./view1";
import { View2Component } from "./view2";
import { MasterGuard, GUARDS } from "./master-guard";
export const routes: Route[] = [
    {
        path: "view1",
        component: View1Component,
        //attach master guard here
        canActivate: [MasterGuard],
        //this is the data object which will be used by 
        //masteer guard to execute guard1 and guard 2
        data: {
            guards: [
                GUARDS.GUARD1,
                GUARDS.GUARD2
            ]
        }
    },
    {
        path: "view2",
        component: View2Component,
        //attach master guard here
        canActivate: [MasterGuard],
        //this is the data object which will be used by 
        //masteer guard to execute guard1, guard 2, guard 3 & guard 4
        data: {
            guards: [
                GUARDS.GUARD1,
                GUARDS.GUARD2,
                GUARDS.GUARD3,
                GUARDS.GUARD4
            ]
        }
    }
];

4. Master Guard -

import { Injectable } from "@angular/core";
import { CanActivate, ActivatedRouteSnapshot, RouterStateSnapshot, Router } from "@angular/router";

//import all the guards in the application
import { Guard1 } from "./guard1";
import { Guard2 } from "./guard2";
import { Guard3 } from "./guard3";
import { Guard4 } from "./guard4";

import { Guard4DependencyService } from "./guard4dependency";

@Injectable()
export class MasterGuard implements CanActivate {

    //you may need to include dependencies of individual guards if specified in guard constructor
    constructor(private _Guard4DependencyService:  Guard4DependencyService) {}

    private route: ActivatedRouteSnapshot;
    private state: RouterStateSnapshot;

    //This method gets triggered when the route is hit
    public canActivate(route: ActivatedRouteSnapshot, state: RouterStateSnapshot): Promise<boolean> {

        this.route = route;
        this.state = state;

        if (!route.data) {
            Promise.resolve(true);
            return;
        }

        //this.route.data.guards is an array of strings set in routing configuration

        if (!this.route.data.guards || !this.route.data.guards.length) {
            Promise.resolve(true);
            return;
        }
        return this.executeGuards();
    }

    //Execute the guards sent in the route data 
    private executeGuards(guardIndex: number = 0): Promise<boolean> {
        return this.activateGuard(this.route.data.guards[guardIndex])
            .then(() => {
                if (guardIndex < this.route.data.guards.length - 1) {
                    return this.executeGuards(guardIndex + 1);
                } else {
                    return Promise.resolve(true);
                }
            })
            .catch(() => {
                return Promise.reject(false);
            });
    }

    //Create an instance of the guard and fire canActivate method returning a promise
    private activateGuard(guardKey: string): Promise<boolean> {

        let guard: Guard1 | Guard2 | Guard3 | Guard4;

        switch (guardKey) {
            case GUARDS.GUARD1:
                guard = new Guard1();
                break;
            case GUARDS.GUARD2:
                guard = new Guard2();
                break;
            case GUARDS.GUARD3:
                guard = new Guard3();
                break;
            case GUARDS.GUARD4:
                guard = new Guard4(this._Guard4DependencyService);
                break;
            default:
                break;
        }
        return guard.canActivate(this.route, this.state);
    }
}

Challenges

One of the challenges in this approach is refactoring of existing routing model. However, it can be done in parts as the changes are non-breaking.

I hope this helps.

Fonseca answered 7/12, 2017 at 15:7 Comment(5)
Thanks a lot for the feedback. I added that link because the other question was answered by me and then I found this question which is about the same problem. I will copy the answer and paste it here to have a better understanding. :)Fonseca
It'd be great if a downvote is supported by a comment explaining the reason so that we can improve ourselves while answering questions. May I know what was the problem with this answer?!Fonseca
This is by far the most comprehensive solution - I am using it!Amenra
Even though I find your solution quite elegant, should it be return Promise.resolve(true); instead of Promise.resolve(true); return; In your current scenario the Promise.resolve has no impact it seems.Prothalamion
Very interesting approach but I see a problem here: the master guard imports all 4 guards, even in a case, when used only two of them, is there an updated version to use/import only provided in the data attribute guards, instead of importing/using all of them in the master guard?Lorenzen
P
23

This issue is resolved in Angular 7.1 and above.

Guards have now a sense of priority.
A detailed explanation on how it works can be found here in this great blog post.

I quote the following example from the blog post:

canActivate: [CanActivateRouteGuard, CanActivateRouteGuard2], 

Which will be working as follows:

All guards in a given canActivate array are executed in parallel, but the router will wait until any guards with a higher priority to finish before moving on. So in the above example:

  • Even if CanActivateRouteGuard2 returns a UrlTree immediately:
    the router will still wait for CanActivateRouteGuard to resolve before initiating a new navigation.
  • If CanActivateRouteGuard returns a UrlTree:
    that will win.
  • If it returns false:
    the entire navigation fails (and no redirects happen).
  • If it simply returns true:
    then the UrlTree returned by CanActivateRouteGuard2 will be navigated to.

UPDATE:

The main issue with the OP is that in the example code route.navigate is called from within both route guards and then a promise resolving to false is returned.

AuthGuard:

...
this.router.navigate(['/login']);
resolve(false);

RoleGuard:

...
this.router.navigate(['/unauthorized']);
resolve(false);

If you want to redirect from a route guard, you are NOT supposed to use route.navigate. Route guards run in parallel meaning that if you call route.navigate multiple times during navigation (during route guard execution) you create a "race" between the different navigation calls and you can never be sure that you will end up on the desired url. Instead of route.navigate you SHOULD return a route UrlTree with the desired redirect. The first returned UrlTree from the guards will be respected and the other redirects will not have any effect.

According to the official documentation for CanActivate in route guards You can return three types of values (optionally wrapped in a Promise or Observable) from a route guard:

  • true : navigation continues.
  • false : navigation is cancelled.
  • UrlTree : the current navigation is cancelled and a new navigation begins to the UrlTree returned from the guard.

In other words calling route.navigate to redirect AND returning false inside the same guard does actually not make sense. The code in the OP above should be changed as follows:

AuthGuard:

...
return this.router.createUrlTree(['/login']);

RoleGuard:

...
return this.router.createUrlTree(['/unauthorized']);

See for an in depth explanation on the internal workings (sense of priority) when using multiple guards the other part of my answer above this update.


Summary: Never call route.navigate inside your guards, return true, false or a UrlTree with the desired redirect.

Proudfoot answered 18/9, 2020 at 12:0 Comment(2)
Thanks for posting this follow-up, using these priorities is a much easier solution.Really
I'm using this solution, when I have 1 guard, all works fine, when I have 2, for some reasson ONLY WHEN PRESSING F5 it redirects me to the default page like if it failed, but when I debug the CanActivate, both returns true... so something wrong is happening with this approach. When navigating with links, this issue is not present.Berley
L
20

As of Angular 8 I am able to do this. This solution was inspired by @planet_hunter's answer but with less code and uses observables for the heavy lifting which was a requirement for this project.

Create a guard with your name of choice that will handle running all guards in order.

@Injectable({
    providedIn: 'root'
})
export class SyncGuardHelper implements CanActivate {
    public constructor(public injector: Injector) {
    }
    canActivate(route: ActivatedRouteSnapshot, state: RouterStateSnapshot): Observable<boolean | UrlTree> {
        return from(route.data.syncGuards).pipe(concatMap((value) => {
            const guard = this.injector.get(value);
            const result = guard.canActivate(route, state);
            if (result instanceof Observable) {
                return result;
            } else if (result instanceof Promise) {
                return from(result);
            } else {
                return of(result);
            }
        }), first((x) => x === false || x instanceof UrlTree, true));
    }
}

In your routes file use the data property to add the guards you want to run in order (synchronously):

const routes: Routes = [
    {
        path: '',
        component: MyComponent,
        canActivate: [SyncGuardHelper],
        data: {
            syncGuards: [
                Guard1,
                Guard2,
                Guard3
            ]
        }
    },
    // other routes
]

I had to come up with this solution today so if you have any feedback please leave a comment so I can improve this answer.

Lille answered 27/11, 2019 at 20:30 Comment(1)
That's a clean solution! Does it break when you use the SyncGuardHelper on both a parent and a child route? I feel like the syncGuards array would be overridden by the child route, and that would result in the parent guards not being executed, and the child guards being executed twice. I might be wrong though!Gerick
P
16

Currently having multiple async guards(returning Promise or Observable) will run at the same time. I opened a issue for this: https://github.com/angular/angular/issues/21702

Another workaround to the described solution above is to use nested routes:

{
  path: '',
  canActivate: [
    AuthGuard,
  ],
  children: [
    {
      path: '',
      canActivate: [
        RoleGuard,
      ],
      component: YourComponent
      // or redirectTo
      // or children
      // or loadChildren
    }
  ]
}
Puritanism answered 22/1, 2018 at 17:11 Comment(3)
This is super clean and very understandable wow. @mick great work.Khoisan
Worked great for me, the empty paths were a little confusing to me so here's an example with a path: const routes: Routes = [ { path: '', component: SummaryComponent, canActivate: [MsalGuard] }, { path: 'administration', canActivate: [MsalGuard], children: [{ path: '', component: AdministrationComponent, canActivate: [RoleGuard], data: { roles: ['Administrator'] } }] }, { path: 'unauthorized', component: UnauthorizedComponent } ];Agar
This answer deserves more votes. It is the simplest and most Angular like solution to add a hierarchy to your route config, and hence take control over the route guards execution order.Proudfoot
B
3

I didn't find a better solution on the internet, but, using as guide the best answer I decide to use only one guard including both requests concatenated using Rxjs mergeMap, this to avoid duplicated calls to the same endpoint. Here my example, avoid the console.log if you want to, I was using it to be sure of what is been triggered first.

1 getCASUsername is called to authenticate the user (heres a console.log(1) that you can't see)
2 We have the userName
3 Here I'm doing a second request that will be triggered after the first one using the response (true)
4 Using the returned userName I get the roles for that user

With this I have the solution for call sequence and for avoiding duplicated calls. Maybe it could work for you.

@Injectable()
export class AuthGuard implements CanActivate {
  constructor(private AuthService  : AuthService,
              private AepApiService: AepApiService) {}

  canActivate(): Observable<boolean> {
    return this.AepApiService.getCASUsername(this.AuthService.token)
      .map(res => {
        console.log(2, 'userName');
        if (res.name) {
          this.AuthService.authenticateUser(res.name);
          return true
        }
      })
      .mergeMap( (res) => {
        console.log(3, 'authenticated: ' + res);
        if (res) {
          return this.AepApiService.getAuthorityRoles(this.AuthService.$userName)
            .map( res => {
              console.log(4, 'roles');
              const roles = res.roles;

              this.AuthService.$userRoles = roles;

              if (!roles.length) this.AuthService.goToAccessDenied();

              return true;
            })
            .catch(() => {
              return Observable.of(false);
            });
        } else {
          return Observable.of(false);
        }
      })
      .catch(():Observable<boolean> => {
        this.AuthService.goToCASLoginPage();
        return Observable.of(false);
      });
  }
}
Budweis answered 26/9, 2017 at 23:35 Comment(0)

© 2022 - 2024 — McMap. All rights reserved.