NavGuards with ionoic-angular 3.5.0 has a breaking change

[mburger81]

Ionic version: (check one with "x") [ ] 1.x (For Ionic 1.x issues, please use https://github.com/ionic-team/ionic-v1) [ ] 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: We are testing your nightly lazy loading implementation which by the way works much better, a big thx to Dan. :+1: On our IonPage's we use the ionViewCanEnter livecycle to check if user can access the Page, if not we will redirect them to a FORBIDDEN PAGE.

This is our simple example, nothing magically

@IonicPage({
    priority: 'off'
})
@Component({
    selector: 'pageForwardToPage4',
    templateUrl: 'pageForwardToPage4.html'
})
export class PageForwardToPage4 {

    constructor(public navCtrl: NavController) {
        console.log('PageForwardToPage4#constructor');
    }

    ionViewCanEnter(): Promise<any> {
        console.log('PageForwardToPage4#ionViewCanEnter');

        return new Promise((resolve, reject) => {
                this.navCtrl.setRoot('Page4');

                console.log('PageForwardToPage4#ionViewCanEnter DO NOT ENTER');

                // THIS DOES WORK page is redirct to Page4
                resolve();

                // THIS DOES NOT WORK, page is will NOT be redirect to Page4 and remain on the same page
                // reject()
        }); 
    }

}

Anytime we get into PageForwardToPage4 we will redirect user to Page4 which should be our ForbiddenPage (this are only dummy names) :smile:

Expected behavior: In older version we used before we reject the promise for the guard, and the redirection was always working. Now if we reject the promise the setRoot doesn't work, so we have to resolve the promise, so user theoretically can access page and will be redirect in the same moment to new page.

So IMO there are three possibilities: 1) this is a bug and should not by design. 2) there is a better way redirect user to forbidden page and also guarantee user can not access the page 3) this is by design, but IMO it's a little bit confusing and as wrote I think this behavior changed after refactoring lazy loading, but this would be a breaking change

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

global packages:

    @ionic/cli-utils : 1.4.0
    Cordova CLI      : 7.0.1 
    Ionic CLI        : 3.4.0

local packages:

    @ionic/app-scripts              : 1.3.8
    @ionic/cli-plugin-cordova       : 1.4.0
    @ionic/cli-plugin-ionic-angular : 1.3.1
    Cordova Platforms               : android 6.2.3
    Ionic Framework                 : ionic-angular 3.4.2-201706271925

System:

    Node       : v6.11.0
    OS         : Linux 4.10
    Xcode      : not installed
    ios-deploy : not installed
    ios-sim    : not installed
    npm        : 5.0.4 

[jgw96]

Thanks for using Ionic, we are looking into this now. Did this behavior work correctly in 3.4.2 for you all?

[mburger81]

Honestly we had (as you perhapse can remebmer) many problems with lazy loading and we was not able to migrate from version 3.0.1´up. So our version in production is and was 3.0.1. On any release, we tested it but there where always problems and we could not upgrade.

After Dans refecatoring today we decided to start with the migartion to the nightly version and try to figure out possible bugs.

And this is the first we think have found.

So we don't really know on which version this "breaking change" was introduced, but If you like we can test it tomorrow morning.

[jgw96]

Thanks for the info (: So yeah, i remember the issues you all were running into and this is partly what has driven us to start making these nav changes. I discussed this issue yesterday with @manucorporat , and it seems that this may not be a bug, but a breaking change that was introduced in 3.x ( I could be wrong here). @manucorporat , would you mind expanding on this please?

[mburger81]

Okay, this breaking change would not be a problem for us, but we think this is not so beautiful. There should be the possibility to redirect user to other page if guard blocking, but if for doing this we have to resolve the promise, so this someone could be see also as a security issue. IMO thx

[mburger81]

Just a comment: The problem is now not only on nightly build problem but it is also on stable version 3.5.0.

Also I did a forum post to find a workaround for that. https://forum.ionicframework.com/t/navguards-with-ionic-angular-3-5-0-has-a-breaking-change/96863/4

[mburger81]

Some help or workaround on this breaking change?

[mburger81]

Yesterday Mike told me we have to do the redirection on pushing the page like this:

this.nav.setRoot(pageName)
              .then((permission) => {
                  if (!permission) {
                      this.nav.setRoot('LoginPage');
                  }
              });

This is working but only a part of the solutions, what if you go direct in the browser to the link, you'll never redirect to the LoginPage, but I think this is a possible scenario, we have and probably also others.

[jgw96]

Hello @mburger81 , sorry for the delay on this. Can you update to 3.5.1 and see if your still running into that last issue after you refactored to use mikes code?

[mburger81]

Hello @jgw96, first of all thx you for your response. At the moment there is no 3.5.1 version released. I already tested it in the last days with all your nightly builds. The thing is, the solution of Mike is probably working now and was working always before.

I think this solution is not working well with the use of deep links, it is working great for a classical android app, but if you use it for webapps you can go directly to an url, where this.nav.setRoot(pageName) is never called and you will stay on a white page and never redirect to loginpage. For this scenario IMO there are guards, if you use an stupid android MENU you can always check on the click event if there are enaught rights, the guards are something like more general, can I or can I not acccess the PAGE. If you can't, and you go direct to page traugh URLs, what you want to do, and don't forget you never have used the NavController to check.

Also in guards there would be usefull more then only a boolean status. In some cases accessing to a Page would not be enaugh have true or false. for example if you are not logged on you would redirect to login page, but if you are logged on but have not the right to access page you would stay on actual page. IMO you have to handle all this in the guards and have the possibility to do anything you wan't to do, and indipendent from using MENUS, URL's, NAVCONTROLLER or whatever else.

IMO thx Michael

[mburger81]

@jgw96 Can you please help? It would be nice to go further with this problem, at least it would be nice to understand. What can we expect and what not?

IMO opinion there is an other problem, as mentioned by mike and others it seems the team would deprecate using Promises on the Guards, he told we have to use boolean instead of Promise. But honestly, why that?? What if the guard is calling an asynch method for example an http request like we do, we can not handle this with an boolean, the only way to do this is a Promise.

Thx Michael

[Barryrowe]

@mburger81 Thanks for pointing me over to this discussion.

I think a lot of the issues you are running into are due to the design issues I was trying to bring up in #11459

Specifically the deeplinking scenarios, and every page essentially having to maintain it's own redirection knowledge, instead of that living in one place.

I have not been through all of the changes in 3.5.x, so I will take a look at those as well. It sounds like from this issue that there may have been some steps forward, but nothing addressing the more global navigation needs of guards and actually preventing a route from loading/instantiating a page altogether.

[mburger81]

@Barryrowe yes I know, I opened several issues about it. IMO the LazyLoading router at the moment is working better then ever. But IMO there are still some design issues like the guards pattern. I don't know what we can expect in the feature for it, and that's one of the main problems. Also I wasn't able until now to figure out how I can workaround all this problems.

[danbucholtz]

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

Thanks, Dan

[brassier]

Curious if there has been any progress on this nav guard issue? We also need to navigate from within our nav guard in certain scenarios.

[olegdater]

All the problems with navigation in Ionic are really frustrating. We are on the edge of abandoning Ionic framework entirely because of the constant problems with navigation/routing ;(

[mburger81]

@brassier @olegwn was you able to figure out some workarounds or something else?

@danbucholtz you wrote you'll have a look soon on this. One question, would we ever have a change to get some workaround or some solution on this problem? IMO I think you all are working hard on ionic4 where you'll introduce a new router, right? So I think probably we never will have a solution for this. right?

It would be good to have at least a status update on this behavior. thx

[brassier]

I landed on a less-than-perfect solution. I have a common class that implements ionViewWillEnter(), and has my guard/redirect logic there. Then all my relevant pages extend this and pick up the event implementation for free.

It works, but the one down side is the page does get instantiated, and there is some visual delay where you can see the blocked page started displaying before it redirected (if it got blocked). Not bad enough for me to find a different solution though.

[mburger81]

Ok but isn't this completely the same result then returning always true to the guards and redirect page if it should be false? We are doing so for now from a while. But in this case if the page is loading faster then the guard (which gets asynch username from server), you'll see the page loaded for x time. But this for now is our solution.

I think there could be also another approach to handle this, with an user observer which handles the redirection. but not sure if this at the end would work!

[brassier]

Yes - similar result as always returning true from the guard. The one benefit is our security check is in one common location and automatically picked up by pages that extend this class.

[mburger81]

We have a AuthGuardService which returns true or false to page event, and in this canActivate method we redirect user to Login or Main page and return always true!

I think this is the standard approach we should us in Ionic https://devdactic.com/ionic-auth-guards/ IMO this is not a real good guard to be flexible.

With the guards how they are now, and consenting again page redirects on returning false as before can do 95% anything you need.

[mburger81]

One of the problems doing something like this:

this.navCtrl.push(SecondPage).catch(err => {
   REDIRECT TO LOGIN PAGE
});

this is working great for APP MENU but it does not work for DEEP LINKING / URLS like webapps and I think also not on PWA's

[mburger81]

@kensodemann can you please jump into this issue? It would be nice to know, if this would be resolved or not, on 18 July an wrote he is going to take a look at nav guards soon..

So for now we don't know if this breaking change would be resolved or not.

If not we would like to figure out with you how can we do a good router using IonicPage and lazyloading with guards. PLZ

best regards Michael

[kensodemann]

@mburger81 - we have a list of high priority / high impact issues to look at when we plan our releases and that this issue is on that list. There are a lot of navigation and lifecycle event related items on there. I can't say if this will make it into the next round or not, but I have it marked as something to discuss when we have our next planning meeting, which will be next Froday.

[HugoHeneault]

So, what happened at this friday @kensodemann ? 🙂

[kensodemann]

@HugoHeneault - This did not make it into the release. At this time, all effort is being put towards the Ionic v4 release, which will include major changes to the navigation system. What those are is still a work in progress.

[iamjoyce]

For anyone who is looking for a workaround, wrap your this.navCtrl.setRoot(...) in a timeout

ionViewCanEnter(){
    if (!this.authService.authenticated){
        setTimeout(() => this.navCtrl.setRoot('login'))
    }
    return this.authService.authenticated,
 }

Source: Stackoverflow

[mburger81]

@kensodemann As in ionic4 you removed ionViewCanEnter and ionViewCanLeave and anther the hood you are using the angular router, I think now we should use default angular guards, with default router redirection. Is my guess right? If yes, I assume this ISSUE is obsolete.

If you like tomorrow I can test if the behavior explained in this issue using the new nav system and guard system, is resolved and in this case the issue would be obsolete.

BTW: do you think you can add some examples for guards, redirect and user login/logout behavior on ionic-conference sample app? The app does not show this best practice for doing this.

thx

[abdel-ships-it]

@mburger81 I think there are plenty of examples out there on how authGuards work! A great example is this repo

[ionitron-bot[bot]]

Thanks for the issue! We have moved the source code and issues for Ionic 3 into a separate repository. I am moving this issue to the repository for Ionic 3. Please track this issue over there.

Thank you for using Ionic!

[ionitron-bot[bot]]

Issue moved to: https://github.com/ionic-team/ionic-v3/issues/30