Angular Guards - Firebase loggedInAndVerified

I have an app that I want to limit to both logged in and verified users. I was able to make two separate guards (logged-in.guard.ts and verified.guard.ts), but since one guard was dependant on the other guard, I made a third guard that combined the two (logged-in-and-verified.guard.ts). I appreciate any feedback, because I am new to angular guards; specifically, is there a better way to do this? It seemed a bit convoluted to me.

The code does work currently.

logged-in.guard.ts

import {Injectable} from '@angular/core';
import {CanActivate, ActivatedRouteSnapshot, RouterStateSnapshot, Router} from '@angular/router';
import {Observable} from 'rxjs/Observable';
import {AngularFireAuth} from 'angularfire2/auth';
import * as firebase from 'firebase/app';

@Injectable()
export class LoggedInGuard implements CanActivate {
  user: Observable;

  constructor(private auth: AngularFireAuth, private router: Router) {
    this.user = auth.authState;
  }

  canActivate(next: ActivatedRouteSnapshot,
              state: RouterStateSnapshot): Observable {
    const url = state.url; // store current url
    return this.checkLogin();
  }

  checkLogin(): Observable {
    let loggedIn;
    return this.user.map(u => {
      loggedIn = !!u; // if u, return true, else return false
      if (loggedIn) {
        return true;
      } else {
        // re-route them to the login page
        this.router.navigate(['/login']);
        return false;
      }
    });
  }
}

verified.guard.ts

```
import {Injectable} from '@angular/core';
import {CanActivate, ActivatedRouteSnapshot, RouterStateSnapshot, Router} from '@angular/router';
import {Observable} from 'rxjs/Observable';
import {AngularFireAuth} from 'angularfire2/auth';
import {AngularFireDatabase} from 'angularfire2/database';

@Injectable()
export class VerifiedGuard implements CanActivate {
loggedIn: boolean;

constructor(private afAuth: AngularFireAuth, private db: AngularFireDatabase) {
}

canActivate(next: Act

Overall, code does not look very bad, especially for the beginner. There are a few things (some are stylistic in nature) which may help with readability and therefore with maintenance.

Misc

const url = state.url; // store current url

• Declares and assigns a const variable that is not being used. Get rid of this line.

checkLogin()

checkLogin(): Observable {
  let loggedIn;
  return this.user.map(u => {
    loggedIn = !!u;
    if (loggedIn) {
      return true;
    } else {
      // re-route them to the login page
      this.router.navigate(['/login']);
      return false;
    }
  });
}

• !! is not an anti-pattern in TypeScript per se, but it's very controversial.

• loggedIn should not be declared in map() outer scope.

• The return observable of this.router.navigate(...) is not being used, I am not sure if you should use it at all (probably, you're good).

Propose:

checkLogin(): Observable {
  return this.user.map(u => {
    if (u != null) {
      return true;
    } else {
      // re-route them to the login page
      const navigationResult = this.router.navigate(['/login']);
      return false;
    }
  });
}

checkVerified()

• Does not declare a return type explicitly, but it should -- TypeScript is about types.

• Using early guards will help reduce nesting, i.e. !this.afAuth.auth.currentUser can be checked to make a return decision right on function entry.

• The code can be compacted by using string interpolation and ternary operator.

Propose:

checkVerified(): Observable {
  if (!this.afAuth.auth.currentUser)
    return Observable.of(false);

  return this.db
    .object(`/users/${this.afAuth.auth.currentUser.uid}`)
    .map(userObj => userObj.$exists() ? userObj.verified : false);
}

checkLoginAndVerified()

• Same things as above apply to checkLoginAndVerified(...).

• .map(loggedInRes => {{ return loggedInRes; }) is doing nothing, therefore can be omitted.

• IMO, reformatting code would make it more readable (this is actually how I noticed the redundancy of that map(...) I just mentioned.

• loggedInVal and verifiedRes should spell the things out.

• BUG? I think the code mistakenly does not use verifiedRes. As far as I understand, it should be used as a condition to this.router.navigate(['/unverified']).

Propose:

checkLoginAndVerified(next, state): Observable {
  return this._loggedInGuard
    .canActivate(next, state)
    .mergeMap(loggedInVal => {
      if (!loggedInVal)
        return Observable.of(false);

      this._verifiedGuard.loggedIn = loggedInVal;
      return this._verifiedGuard
        .canActivate(next, state)
        .map(verifiedRes => {
           if (!verifiedRes) {
             this.router.navigate(['/unverified']);
           }
           return verifiedRes;
        });
    });
}

StackExchange Code Review Q#162995, answer score: 2