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