3
\$\begingroup\$

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
\$\endgroup\$
2
\$\begingroup\$

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;
        });
    });
}
\$\endgroup\$
0
\$\begingroup\$

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

  }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.