Angular Guards - Firebase loggedInAndVerified

Question

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<firebase.User>;

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

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

  checkLogin(): Observable<boolean> {
    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: ActivatedRouteSnapshot,
              state: RouterStateSnapshot): Observable<boolean> {
    if (!this.loggedIn) { // this Guard is always dependant of being logged in
      return Observable.of(false);
    }
    return this.checkVerified();
  }


  checkVerified() {
    if (this.afAuth.auth.currentUser) {
      const uid = this.afAuth.auth.currentUser.uid;
      return this.db.object('/users/' + uid).map(userObj => {
        if (userObj.$exists()) {
          return userObj.verified;
        } else {
          return false;
        }
      });
    }
    return Observable.of(false);

  }
}

logged-in-and-verified.guard.ts

import {Injectable} from '@angular/core';
import {CanActivate, ActivatedRouteSnapshot, RouterStateSnapshot, Router} from '@angular/router';
import {Observable} from 'rxjs/Observable';
import {LoggedInGuard} from './logged-in.guard';
import {VerifiedGuard} from './verified.guard';
import 'rxjs/add/operator/mergeMap';

@Injectable()
export class LoggedInAndVerifiedGuard implements CanActivate {
  constructor(private _loggedInGuard: LoggedInGuard, private _verifiedGuard: VerifiedGuard,
              private router: Router) {
  }

  canActivate(next: ActivatedRouteSnapshot,
              state: RouterStateSnapshot): boolean | Observable<boolean> | Promise<boolean> {
    return this.checkLoginAndVerified(next, state);
  }

  checkLoginAndVerified(next, state) {

    return this._loggedInGuard.canActivate(next, state).map((loggedInRes) => {
      return loggedInRes;
    }).mergeMap(loggedInVal => {
      if (loggedInVal) {
        this._verifiedGuard.loggedIn = loggedInVal;
        return this._verifiedGuard.canActivate(next, state).map((verifiedRes) => {
          this.router.navigate(['/unverified']);
          return verifiedRes;
        });
      } else {
        return Observable.of(false);
      }
    });
  }
}

version info (in case it matters)

@angular/cli: 1.0.0
node: 7.7.3
os: darwin x64
@angular/animations: 4.1.0
@angular/common: 4.1.0
@angular/compiler: 4.1.0
@angular/core: 4.1.0
@angular/forms: 4.1.0
@angular/http: 4.1.0
@angular/material: 2.0.0-beta.3
@angular/platform-browser: 4.1.0
@angular/platform-browser-dynamic: 4.1.0
@angular/router: 4.1.0
@angular/cli: 1.0.0
@angular/compiler-cli: 4.1.0

Answer 1

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<boolean> {
  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<boolean> {
  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<boolean> {
  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<boolean> {
  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;
        });
    });
}

Answer 2

I found another answer here which leads me to this.

1) Since I am only referring to the current user, if this user is verified, it can be implied they are logged in.

2) I can use the angularfire2 authstate to simplify further.

my new verified.guard.ts file:

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 {

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


  canActivate(next: ActivatedRouteSnapshot,
              state: RouterStateSnapshot): Observable<boolean> {

    return new Observable<boolean>(observer => {
      this.afAuth.authState.subscribe((authState) => {
        if (authState) {
          this.checkVerified().subscribe((adminState) => {
            observer.next(adminState);
          });
        } else {
          this.router.navigate(['/login']);
          observer.next(false);
        }
      });
    });
  }


  checkVerified() {
    if (this.afAuth.auth.currentUser) {
      const uid = this.afAuth.auth.currentUser.uid;
      return this.db.object('/users/' + uid).map(userObj => {
        if (userObj.$exists()) {
          if (userObj.verified) {
            return true;
          } else {
            this.router.navigate(['/unverified']);
            return false;
          }
        } else {
          return false;
        }
      });
    }
    return Observable.of(false);

  }
}