Description

blindstuff

opened on Nov 10, 2018

Issue body actions

• I understand that GitHub issues are not for tech support, but for questions specific to this generator, bug reports, and feature requests.

Item	Version
generator-angular-fullstack	5.0.0 rc 4 + fix PRs
Node	8.10.0
npm	5.7.1
Operating System	Windows 10

Item	Answer
Transpiler	TypeScript
Markup	HTML
CSS	SCSS
Router	ngRoute
Client Tests	Mocha
DB	SQL
Auth	Y

A user that has sucesfully logged on will not be detected as logged in if the site is refreshed inside an AuthGuarded route.

Steps to reproduce bug:

1. Use generator
2. Log in as admin user
3. Click on Admin tab on nav bar
4. Use browser refresh

Cause:
Router is evaluating the CanActivate function before the constructor has retrevied the user from the Token

AuthGuard triggered

canActivate() {
	return this.authService.isLoggedIn();
}

isLoggedIn(callback?) {
	let is = !!this.currentUser._id;
	//at this time is continues to be undefined
	safeCb(callback)(is);
	return Promise.resolve(is);
}

AuthGuard Cancels Navigation

This part of the constructor finishes running and get the user too late.

if(localStorage.getItem('id_token')) {
	this.UserService.get().toPromise()
		.then((user: User) => {
			this.currentUser = user;
		})
		.catch(err => {
			console.log(err);

			localStorage.removeItem('id_token');
		});
}

Click on another authguarded route after this will work. Bug only occurs while refreshing.

Activity

1. blindstuff commented on Nov 12, 2018

   

   blindstuff

   on Nov 12, 2018

   Author

   More actions

   Implemented the following solution, will submit a PR if in agreement with strategy. @Awk34

   auth.service.ts:

   constructor(private http: HttpClient, private userService: UserService) {
   	this.http = http;
   	this.UserService = userService;
   
   	//Not calling this as it will get done the first time isLoggedIn() is called
           //TODO: Delete this comment block. Currently commented for clarity.
   	/*
   	if(localStorage.getItem('id_token')) {
   		this.UserService.get().toPromise()
   			.then((user: User) => {
   				this.currentUser = user;
   			})
   			.catch(err => {
   				console.log(err);
   
   				localStorage.removeItem('id_token');
   			});
   	}
   	*/
   }
   
   isLoggedIn(callback?) {
   	let is = !!this.currentUser._id;
   	if(is || !localStorage.getItem('id_token')){
   		//Got the user locally, or there is no id_token stored
   		safeCb(callback)(is);
   		return Promise.resolve(is);
   	}else{
   		//User not yet loaded locally, but id_token is present
   		return this.UserService.get().toPromise()
   			.then((user: User) => {
   				this.currentUser = user;
   				return !!this.currentUser._id;
   			})
   			.catch(err => {
   				console.log(err);
   				localStorage.removeItem('id_token');
   				return false;
   			});
   	}
   }
   

   auth-guard.service.ts:

   //Change can activate to return a Promise
   canActivate(): Promise<boolean> {
   	return this.authService.isLoggedIn();
   }
   

   Reacted by albert-92
2. koraysels commented on Nov 14, 2018

   

   koraysels

   on Nov 14, 2018

   More actions

   Definitely an improvement, this works pretty good for me. Thanks!
3. albert-92 commented on Dec 10, 2018

   

   albert-92

   on Dec 10, 2018

   Contributor

   More actions

   Cool solution @blindstuff (#2774 (comment))

   How about taking it even a little bit further like:

   auth.service.ts:

   hasRolePromise(role, callback?) {
       let is = !!this.currentUser._id;
       if(is || !localStorage.getItem('id_token')) {
           //Got the user locally, or there is no id_token stored
           safeCb(callback)(is);
           return Promise.resolve(is);
       } else {
           //User not yet loaded locally, but id_token is present
           return this.UserService.get().toPromise()
               .then((user: User) => {
                   this.currentUser = user;
                   return this.hasRole(user.role, role)
               })
               .catch(err => {
                   console.log(err);
                   localStorage.removeItem('id_token');
                   return false;
               });
       }
   }
   

   auth-guard.service.ts:

   static parameters = [Router, AuthService];
   constructor(
       private router: Router,
       private authService: AuthService
   ) {
       this.authService = authService;
       this.router = router;
   }
   
   canActivate(route: ActivatedRouteSnapshot): Promise<boolean> {
       return this.authService.hasRolePromise(route.data.role).then(is => {
           if (!is) {
               this.router.navigate(['/login']);
           }
           return is;
       });
   }
   

   and for example in the admin module
   admin.module.ts:

   const adminRoutes: Routes = [{
       path: 'admin',
       component: AdminComponent,
       canActivate: [AuthGuard],
       data: {role: 'admin'}
   }];
   

   Reacted by Stefano
4. blindstuff commented on Dec 19, 2018

   

   blindstuff

   on Dec 19, 2018

   Author

   More actions

   Good idea @albert-92

   I had solved this in my implementation by editing canActivate. Either route seems to do well, I preferred to keep them separate, but am open to either.

   canActivate(route: ActivatedRouteSnapshot): Promise<boolean> {
           // this will be passed from the route config
           // on the data property
           const expectedRole = route.data.expectedRole;
   
           let promise = new Promise<boolean>((resolve,reject) => {
               this.authService.isLoggedIn().then(is => {
                   resolve(this.authService.hasRole(this.authService.currentUser.role,expectedRole));
               });
           });
   
           return promise;
       }
   

   Reacted by albert-92

Issue actions