Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The suggested "guard" approach with ionViewCanEnter/ionViewCanLeave appears to lack configuration, leak objects, and could hide dangerous code. #11459

Closed
Barryrowe opened this issue May 1, 2017 · 21 comments
Labels
ionitron: v3 moves the issue to the ionic-v3 repository

Comments

@Barryrowe
Copy link
Contributor

Ionic version: (check one with "x")
[ ] 1.x
[x] 2.x
[x] 3.x

I'm submitting a ... (check one with "x")
[x] bug report
[ ] feature request
[ ] support request => Please do not submit support requests here, use one of these channels: https://forum.ionicframework.com/ or http://ionicworldwide.herokuapp.com/

Current behavior:

When implementing the suggested "guard" approach for Ionic, using the ionViewCanEnter hook on a page, there are a handful of issues that can arise with its implementation:

Lack Configuration
There is currently no configurable way to tell the navController where to go by default if an ionViewCanEnter returns/resolves false as the first route (a very possible case with DeepLinking). All you can do at the moment is inject navCtrl and manually navigate the user. This is tedious, and error prone when you have many pages that could receive a DeepLink that needs a guard. Being able to set a default redirect for a rejected ionCanViewEnter makes sense. Or maybe ionViewCanEnter needs to return an object like: { canEnter: boolean, redirectPage?: any }, so they can still override it. This is related to #11405

Leak Objects
If the ionViewCanEnter function returns/resolves false the newly instantiated component is never destroyed. (ngOnDestroy is never called).

Hide Dangerous Code
Unlike the core angular router guards, this implementation requires an instance of the component to be instantiated. This means that a developer could write some logic in the constructor for the component that they don't expect to run, that will always run regardless of the result of ionViewCanEnter

This paired with the lack of guaranteeing the components are destroyed, could lead to memory leaks, additional subscriptions, etc. Ex: it's not uncommon for Observable subscriptions to be created in a constructor, and if they aren't unsubscribed, this can prevent the component from being GC'd, as well as potentially duplicating the observable chain logic.

LazyLoading
In addition to the above items, if you're setting up lazy loaded modules with DeepLinking, the way guarding is handles means a whole module bundle has to be requested, loaded, and run before the decision is made that "Hey you shouldn't even be here"

Expected behavior:
I know there are complications with this, but the way the core angular router handles guarding seems to be the correct approach. Guarding at the navigation level and not at the page component level. This is where you have the chance to prevent the component from even being built, or loading a module that hasn't been yet loaded in lazy loading scenarios.

Steps to reproduce:

A simple example of not getting destroyed and not redirecting. Doesn't show the mulitiples as I didn't implement tabs.
http://plnkr.co/edit/Y7LNSD8xy2q6TcPPpXQf?p=preview

To create a more complete example

  1. Create a project with the tabs template

  2. Implement ionCanViewenter in the home page component to return false

  3. Implement ngOnDestroy in the home page to console log when it is destroyed

  4. Add a console.log line to the constructor for the home page to show it's being instantiated.

  5. Run the app, open a console, and see that A) the page doesn't default to loading anything since the tabs page can't load it's view and B) ngOnDestroy is never called on the home page even though it is constructed.

Related code:

ionViewCanEnter(){
 //You can uncomment this to 'redirect' to the About page and see that multiple attempts to navigate to Home will result in multiple constructions and no destroys.
 //this.navCtrl.setRoot(AboutPage);
 return false;
}

ngOnDestroy(){
   console.log("sup nerds? I should be destroyed now right?");
}

Other information:
Ionic info: (run ionic info from a terminal/cmd prompt and paste output below):

Cordova CLI: 6.5.0
Ionic Framework Version: 3.0.1
Ionic CLI Version: 2.2.2
Ionic App Lib Version: 2.2.1
Ionic App Scripts Version: 1.3.0
ios-deploy version: Not installed
ios-sim version: Not installed
OS: Windows 7
Node Version: v7.9.0
Xcode version: Not installed
@jgw96 jgw96 added the v2 label May 1, 2017
@jgw96
Copy link
Contributor

jgw96 commented May 1, 2017

Hello @Barryrowe , thanks for the detailed issue! We will look into this.

@mburger81
Copy link
Contributor

Very good post @Barryrowe and I'm 100% in your opinion.

@olegdater
Copy link

@Barryrowe right to the point. It's our pain to implement route guards with Ionic 3 :( Works fine for small apps, but for production ready - nope

@jsanta
Copy link

jsanta commented Jul 11, 2017

App menus have the same problem.
I expected this kind of code to work:

ionViewCanEnter(): boolean {
   if(!!this.global.get('token')) { // Allow entrance if there is a token (or any other validation)
      return true;
    } 
    
    this.navCtrl.setRoot('LoginPage');    // LoginPage is my rootPage/default
    return false;
  }

it didn't, so I had to use ionWillEnter to make the same validations.
A white screen is presented when using ionic serve, and as the screen has a menu , I can access the menu even though I shouldn't be able to enter the page.

@Barryrowe
Copy link
Contributor Author

@jgw96 I'm curious if this is being looked at? I know there was talk on other threads around the time I opened this that DeepLinking and routing in general was getting a heavy look, and possible restructure, and that would definitely impact this issue.

I haven't seen anything of the sorts, but I may have missed it. Just looking for any feedback the team might have.

Thank you.

@mburger81
Copy link
Contributor

Have a look also to this issue #12193

@danbucholtz
Copy link
Contributor

I am going to take a look at nav guards soon.

Thanks,
Dan

@danbucholtz danbucholtz self-assigned this Jul 18, 2017
@macheema
Copy link

macheema commented Oct 5, 2017

May we return observable as follow??

  ionViewCanEnter() {
     return this.sessionService.sessionEvent$.map(user => {
       this.user = user;
       if (this.user.isAuthenticated == false) {
         return false;
       }
     });
   }

@FazioNico
Copy link

Here my use of ionViewCanEnter().
It work find ;-)

https://github.com/FazioNico/mean-ionic-ngrx/blob/master/src/decorators/index.ts
https://github.com/FazioNico/mean-ionic-ngrx/blob/master/src/pages/home/home.ts

@abdel-ships-it
Copy link

@danbucholtz Any updates on this dan?

@rbaumi
Copy link

rbaumi commented Dec 1, 2017

@danbucholtz Any updates?

@mulish77
Copy link

mulish77 commented Dec 3, 2017

@danbucholtz It will be helpful if you share us how long does it take or your opinion to solve this?

@mburger81
Copy link
Contributor

Not sure if this can help, but #12193 was before in different milestones but never closed, now it is on milestones for 4.x so perhaps we will have it there and perhaps also this could be resolved on ionic4.
I think with version 4 they would do a next step with there router.
For what I know, also they are discussing using the plain angular router, which would be fantastic!
BTW on ionic4 you could use any framework with there component so you could use plain angular framework with ionic web components.
And there is also already now a project which is called ngX-Rocket where you can use ionic with plain angular4 project structure using there router.

We'll wait for ionic4 and see what they w'll do with router and lazyloading, if we are not satisfied we'll use only there web components with ngx-rocket or plain angular project.

IMO

@MaxXx1313
Copy link

It seems like this.navCtrl.setRoot() doesn't work in the same tick, but using setTimeout works fine for me:

  ionViewCanEnter(): boolean | Promise<any> {
    const isAllowed = false; // some expression here

    console.log('TestPage: ionViewCanEnter:', isAllowed);
    if (!isAllowed) {
      // can't navigate in the same tick
      setTimeout(() => {
        console.log('TestPage: redirect');
        this.navCtrl.setRoot('feedback');
      }, 0);
    }
    return isAllowed;
  }

@thegust
Copy link

thegust commented Mar 2, 2018

The above example works fine, but the whole function needs to be replicated in every page.
I think this should be better

In your AuthProvider:

isAuthenticated(nav: NavController): boolean | Promise<any> {
	return this.storage.get("token").then(token => {
		if (token == null) {
			setTimeout(() => { nav.setRoot("SignInPage") }, 0);
			return false
		} else {
			return true
		}
	}).catch(() => {
		setTimeout(() => { nav.setRoot("SignInPage") }, 0);
		return false
	});
}

Now you can just simply call the function in your pages:

ionViewCanEnter(): boolean | Promise<any> {
	return this.auth.isAuthenticated(this.navCtrl);
}

@dalenguyen
Copy link

Hi guys, This is what I did for my firebase project. It works

  ionViewCanEnter(): Promise<any>{
    return new Promise((resolve, reject) => {
      this.auth.getUser().then((user: firebase.User) => {            
        resolve(true);        
      }).catch(() => {
        resolve(false);
      });
    })
  }

@HardikDG
Copy link

HardikDG commented May 19, 2018

@thegust's approach is very good for individual pages. Anyone got any global solution instead of writing the 'ionViewCanEnter' on every page.

@IzioDev
Copy link

IzioDev commented Jun 6, 2018

@HardikDG Wait for Ionic V4 with Angular Routing implementation

@taofik-nos
Copy link

taofik-nos commented Aug 13, 2018

One way is to use async which worked for me.

isAuthenticated() in auth provider:

 isAuthenticated() {
    return new Promise((resolve, reject) => {
      this.storage.get('token').then((token) => {
        if(token)
          resolve(true)
        else
          resolve(false)
        }).catch((e) => {
          resolve(false)
        })
    })
  }

Then using ionViewCanEnter()as follows:

async ionViewCanEnter() {
    const can = await this.authProvider.isAuthenticated();
    
    if(can){
      return false;
    }

    return true;
  }

@adamdbradley adamdbradley added the ionitron: v3 moves the issue to the ionic-v3 repository label Nov 1, 2018
@imhoffd imhoffd removed ionitron: v3 moves the issue to the ionic-v3 repository labels Nov 28, 2018
@Ionitron Ionitron added the ionitron: v3 moves the issue to the ionic-v3 repository label Nov 28, 2018
@ionitron-bot
Copy link

ionitron-bot bot commented Nov 28, 2018

This issue has been automatically identified as an Ionic 3 issue. We recently moved Ionic 3 to its own repository. I am moving this issue to the repository for Ionic 3. Please track this issue over there.

If I've made a mistake, and if this issue is still relevant to Ionic 4, please let the Ionic Framework team know!

Thank you for using Ionic!

@ionitron-bot
Copy link

ionitron-bot bot commented Nov 28, 2018

Issue moved to: ionic-team/ionic-v3#281

@ionitron-bot ionitron-bot bot closed this as completed Nov 28, 2018
@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Nov 28, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ionitron: v3 moves the issue to the ionic-v3 repository
Projects
None yet
Development

No branches or pull requests