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

[Question]: Why is AutoLoginAllRoutesGuard not recommended? #1549

Closed
heapifyman opened this issue Sep 27, 2022 · 22 comments
Closed

[Question]: Why is AutoLoginAllRoutesGuard not recommended? #1549

heapifyman opened this issue Sep 27, 2022 · 22 comments
Labels

Comments

@heapifyman
Copy link
Contributor

What Version of the library are you using?

14.1.5

Question

The documentation for AutoLoginAllRoutesGuard states:

We do NOT recommend all routes to be guarded.

I may have missed it somewhere in the documentation but I could not find any explanation why it is not recommended.
Is the implementation considered unstable or is there maybe some fundamental technical issue?
Could someone provide some details why exactly it is not recommended?
And would it make sense to add that explanation to the above documentation?

The docs continue:

We know that this is a common use case but it is the easiest to have at least one public route, e.g. /callback where your identity provider can redirect to. In this route you can set up the authentication and then redirect to the route wou wanted to go to initially. This would be the "normal" behaviour of an SPA.

Is there an example application available that demonstrates how to do that? And would it make sense to add it to the samples for this project because it is the recommended way of implementing this use case - maybe it would even replace the current sample for AutoLoginAllRoutesGuard?

@FabianGosebrink
Copy link
Collaborator

Hey, thanks for asking. Like written in the docs, and you cited it very correct, an SPA normally needs a point to set up the auth when it gets redirected from the IDP to the app itself. If you think of the state of the app, which is not authorized in this case, but the state of the "system" (Your IDP, maybe backend and SPA) is "currently authorizing". Because you entered the correct credentials and therefore as the user, you should even be "authorized" at this point. So things are a little more complex here. You land in the app with a code and the SPA needs to do a few calls (exchange the code with a token, etc.) but for the app at this point you are NOT authenticated. This is why you technically need a route which is publically accessible without being authenticated to have an entry point to get all the things going. The lib does that for you, if you call the "checkAuth()" method. So in your callback-component (for example) you can call "checkAuth()" and wait for the response coming back. The lib provides a login response then and in positive case you can redirect to your real application, your dashboard, starting page or whatever. So there is like a "handshake" going on between your IDP and your app. It is not just SPA --> IDP --> SPA, like the user sees it.

What we tried to do is covering that case with a guard, because guards are called before the route is being called, and the component is shown. Because the mentioned callback component was not being used for many users of this lib and the question "How can I secure my complete app?" was asked many times. But the guard is only one standard way of "checking and redirecting". We have encountered that we had many "bugs" or better "questions" in exactly that procedure: Auth with many different IDPs, redirecting back and forth to the correct route in every case for every app using this lib. We tried to solve it with this guard. It all could have been solved with this one public route which provides you better control over the step between the "User provided correct credentials and therefore for the USER you are logged in" and the step "The SPA HAS a valid ID- and refresh token, all the timers for refresh are set up, and we are good to go". This is a problem we developers have to solve. The guard was one attempt, but if this is not working in your case, with the docs we wanted to mention the solution with the public route, which is easier in terms of seeing what is going on, because it represents the steps in between better, but of course not that comfortable as just putting a guard on your route, and you're good to go.

On a personal note, I regret implementing this guard because for me, it seems it created more bugs, questions and confusion to the users than it helped them. Just having that one public route would have been the easier way. But you are always smarter in the end :-)

I hope I could help you a bit.

@heapifyman
Copy link
Contributor Author

Thanks for the detailed explanation. But just to be clear: the recommended approach for "I want to secure my complete app" would be:

  • protect all routes with AutoLoginPartialRoutesGuard
  • except for one route /callback (name can be different of course) - and routes like { path: '**', redirectTo: 'home' }
  • call oidcSecurityService.checkAuth in CallbackComponent.onInit and "redirect" in case of successful login?

Unfortunately, I am still struggling to implement something like that. So far I have something that mostly works (see code below). I can login and navigate to all routes. If am not logged in and try to access any of the protected routes the login flow is automatically initiated.

But there are at least 2 cases where it is not working correctly:

  1. When I am already logged in and on the home page localhost:4200/home, then manually paste localhost:4200/next (or any other route) into the browser's address bar and hit enter (or simply press F5), the navigation toolbar (ToolbarComponent) with logout button disappears. But NextComponent is displayed correctly.
  2. When I am not logged in, paste localhost:4200/next into the browser's address bar and hit enter, I end up on the HomeComponent, not the NextComponent.

For the first case it seems that oidcSecurityService.isAuthenticated$ is not emitting the current value (true) again? It seems that I can work around that by calling oidcSecurityService.checkAuth also in my AppComponent.onInit. But that seems redundant because I already have it in CallbackComponent.

And for the second case it seems that the AutoLoginPartialRoutesGuard does not retrieve the correct initial URL from storage again. The docs seem to indicate that I should do that myself:

and then redirect to the route you wanted to go to initially.

But that also seems redundant because the logic for that is already implemented in AutoLoginPartialRoutesGuard.

It would be great if someone could point out what I am missing.

AppRoutingModule:

const routes: Routes = [
  { path: '', pathMatch: 'full', redirectTo: 'home' },
  {
    path: 'home',
    loadChildren: () => import('./home/home.module').then((m) => m.HomeModule),
    canLoad: [AutoLoginPartialRoutesGuard],
  },
  {
    path: 'next',
    loadChildren: () => import('./next/next.module').then((m) => m.NextModule),
    canLoad: [AutoLoginPartialRoutesGuard],
  },
  { path: 'callback', component: CallbackComponent },
  { path: '**', redirectTo: 'home' },
];

@NgModule({
  imports: [RouterModule.forRoot(routes)],
  exports: [RouterModule],
})
export class AppRoutingModule {}

CallbackComponent

@Component({
  selector: 'app-callback',
  template: '',
  styles: [],
})
export class CallbackComponent implements OnInit, OnDestroy {
  private readonly subscription = new Subscription();

  constructor(
    private readonly router: Router,
    private readonly oidcSecurityService: OidcSecurityService
  ) {}

  ngOnInit(): void {
    this.subscription.add(
      this.oidcSecurityService.checkAuth().subscribe(({ isAuthenticated }) => {
        this.router.navigate(['home']);
      })
    );
  }

  ngOnDestroy(): void {
    this.subscription.unsubscribe();
  }
}

ToolbarComponent:

@Component({
  selector: 'app-toolbar',
  templateUrl: './toolbar.component.html',
  styleUrls: ['./toolbar.component.css'],
})
export class ToolbarComponent implements OnInit, OnDestroy {
  readonly title = 'oauth-test';

  readonly isAuthenticated$: Observable<boolean>;

  private readonly subscription = new Subscription();

  constructor(private readonly oidcSecurityService: OidcSecurityService) {
    this.isAuthenticated$ = this.oidcSecurityService.isAuthenticated$.pipe(
      map(({ isAuthenticated }) => isAuthenticated)
    );
  }

  ngOnInit(): void {
  }

  ngOnDestroy(): void {
    this.subscription.unsubscribe();
  }

  logout(): void {
    this.subscription.add(
      this.oidcSecurityService
        .logoffAndRevokeTokens()
        .subscribe(() => console.log('[TOOLBAR] logged out'))
    );
  }
}

toolbar.component.html:

<div *ngIf="isAuthenticated$ | async">
  <h1>{{ title }}</h1>

  <div>
    <button (click)="logout()">Logout</button>
  </div>
</div>

@FabianGosebrink
Copy link
Collaborator

You have to call checkAuth() also when the application starts.

@heapifyman
Copy link
Contributor Author

heapifyman commented Nov 2, 2022

@FabianGosebrink ok, thanks.

Calling checkAuth also in AppComponent.onInit does work.

@KegLand
Copy link

KegLand commented Dec 6, 2022

Is there a working sample for this pattern available? This seems to conflict other information mentioned elsewhere, where you shouldn't call checkAuth() more than once. I don't see how you are supposed to have checkAuth() in the AppComponent.onInit as well as within the callback component without it being called twice.

@jalix219
Copy link

jalix219 commented Dec 8, 2022

I've got the same question as @KegLand. I've implemented the callback component and call checkAuth() in the AppComponent.onInit. I can see the method is being called twice. I'm not sure how you would implement it otherwise though. Any help would be welcomed.

@heapifyman
Copy link
Contributor Author

Calling checkAuth also in AppComponent.onInit does work.

I have to correct myself. Calling checkAuth a second time besides the CallbackComponent seems to cause angular-auth-oidc-client to make two requests to the token endpoint.

This was no problem in my first tests using Azure AD as IDP. It seems that Azure AD ignores the second request.

But when I tested against a Keycloak 20.0.3 Keycloak would return a 400 Bad Request for the second call to the token endpoint. And that caused issues in my app, like redirect to the originally requested page after login was not working properly. Sometimes the user would be redirected to the "home" page instead. Logout and logging in as a different user in the same tab would not work properly, etc.

So it seems that the current version of auto-login for all routes sample, which only calls checkAuth once in the CallbackComponent is the only working version.

However, it still has one flaw, namely the that reloading the page (F5) - or accessing another page by manually changing the URL in the browser's address bar - when already logged in will break the sample's NavigationComponent. After pressing F5 isAuthenticated will be false, the logout link disappears and only "Route to 'protected' Route & Auto Login" will be shown.

It seems that the OidcSecurityService.isAuthenticated$ observable does not emit the correct value after pressing F5. Although, OidcSecurityService.isAuthenticated() will return the correct value.

A workaround could be to change NavigationComponent.ngOnInit like this:

  ngOnInit() {
    this.oidcSecurityService.isAuthenticated$
      .pipe(
        tap(({ isAuthenticated }) => console.log('[NavigationComponent] isAuthenticated$: ', isAuthenticated)),
        concatMap((_) => this.oidcSecurityService.isAuthenticated())
      )
      .subscribe((isAuthenticated) => {
        console.log('[NavigationComponent] authenticated: ', isAuthenticated);
        this.isAuthenticated = isAuthenticated;
      });
  }

This will ensure that NavigationComponent.isAuthenticated will receive the correct value and the browser console will print:

[NavigationComponent] isAuthenticated$:  false
[NavigationComponent] authenticated:  true

@FabianGosebrink why do OidcSecurityService.isAuthenticated$ and OidcSecurityService.isAuthenticated return different values in this case?

Should AutoLoginPartialRoutesGuard.checkAuth maybe call AuthStateService.setAuthenticatedAndFireEvent in if (isAuthenticated) { so that AuthStateService.authenticatedInternal$ gets the correct value?

@heapifyman
Copy link
Contributor Author

However, it still has one flaw, namely the that reloading the page (F5) - or accessing another page by manually changing the URL in the browser's address bar - when already logged in will break the sample's NavigationComponent. After pressing F5 isAuthenticated will be false, the logout link disappears and only "Route to 'protected' Route & Auto Login" will be shown.

It seems that the OidcSecurityService.isAuthenticated$ observable does not emit the correct value after pressing F5. Although, OidcSecurityService.isAuthenticated() will return the correct value.
...

and the browser console will print:

[NavigationComponent] isAuthenticated$:  false
[NavigationComponent] authenticated:  true

In fact, it is not only OidcSecurityService.isAuthenticated$ that emits a wrong value after pressing F5. OidcSecurityService.userDate$ is also affected and does not return any value.
So the samples HomeComponent is also broken when pressing F5.

@alexeek
Copy link

alexeek commented Mar 17, 2023

@FabianGosebrink I have the same issue as the guys have. I don't have public pages except /callback, and in my case, i have the method checkAuth called twice and on the second call I get 400 error (spring oauth2). How we can deal with it?

@simon-d
Copy link

simon-d commented May 9, 2023

Having the same issue as @alexeek when following the advice given here. By having a checkAuth() call in both the app component Init and the public callback component, it results in two requests to the token endpoint. In my case this causes a 400 error result from my IDP (Identity server) and my logic to handle completion of authentication process in the callback component is failing as it is the second request and gets the error result.

Is there any more guidance on how to avoid this duplicate request issue? I've tried not having the checkAuth() call in the app component, but then I've not been able to correctly handle page refreshes when there is an active session.

@FabianGosebrink
Copy link
Collaborator

Try the checkAuth() in the app.component only.

@Anirudh16
Copy link

Hey @FabianGosebrink , facing the same issue with checkAuth() method which i used in callback component and also app component with AutoLoginPartialRoutesGuard

@FabianGosebrink
Copy link
Collaborator

FabianGosebrink commented Jun 22, 2023

Try the checkAuth() in the app.component only.

:)

@milo526
Copy link

milo526 commented Aug 15, 2023

Calling checkAuth() exactly once, only in the app.component.ts does indeed seem to work, but this is not reflected in the sample.

https://github.com/damienbod/angular-auth-oidc-client/blob/8f4553aadd3e54f182a8388a26fbc293913b4745/projects/sample-code-flow-auto-login-all-routes/src/app/app.component.ts

The sample only calls checkAuth() in the callback.component.ts.

@FabianGosebrink
Copy link
Collaborator

You can call this method wherever you want, but it has to be called when you are getting redirected from the sts.

@milo526
Copy link

milo526 commented Aug 15, 2023

However, it still has one flaw, namely the that reloading the page (F5) - or accessing another page by manually changing the URL in the browser's address bar - when already logged in will break the sample's NavigationComponent. After pressing F5 isAuthenticated will be false, the logout link disappears and only "Route to 'protected' Route & Auto Login" will be shown.

I had the same problem as @heapifyman where isAuthenticated$ would return false (whilst I am authenticated) after pressing F5 if I did not call checkAuth() on every load of the application.

So it seems that for my situation (and seemingly others) it is required to call checkAuth() always; not just after getting redirected back from the sts (which would be the case if we only put checkAuth() in the callback.component.ts).

@FabianGosebrink
Copy link
Collaborator

True. My comment is not complete. checkAuth() sets up the authentication when you are getting redirected and when you hit F5, so the app is initialized completely new. It is the entry point of the lib. The method behaves differently whether you are already authenticated, not authenticated at all or have the auth information in the URL, which is during the authentication process. Sorry for being not clear here. Should we improve the documentation to make this more clear?

@philippemarcelino
Copy link

Hi,

I'm currently trying to implement this exact "protect all app" thing using the discussed patterns here.
I basically tried various suggestions and ended up with the same issues already discussed here. Plus some more... Like I observed some weird behaviour where the callback ngOnInit was called before the main app component ngOnInit...

The solution that seems to work best in my case is to put this checkAuth call in the APP_INITIALIZER code portion.
Here, I first make a call to isAuthenticated. Next I can call checkAuth (the only one) and call the resolve callback after all the init stuff is done (both the oidc stuff and my specific app logic).
Seems to work with normal login, normal navigation, F5 and url typing/copy/pasting.

But at this point the actual auth callback public route seems basically useless...

My init code looks like this:

return new Promise<void>((resolve, reject) => {
	this.oidcSecurityService.isAuthenticated().subscribe((isAuthenticated) => {
		this.isAuthenticated = isAuthenticated;
	});

	return this.oidcSecurityService.checkAuth().subscribe((loginResponse: LoginResponse) => {
		const { isAuthenticated, userData, accessToken, idToken, configId } = loginResponse;
		console.debug('App init: isAuthenticated=' + isAuthenticated);
		console.debug('App init: userData=' + userData?.email);

		if (!this.isAuthenticated) {
			// custom app init logic here... sync or async
		}

		resolve();
	});
});

So basically, I think this fixes the issue by forcing the app to wait for the auth handshake stuff to complete before any component is initialized.
@FabianGosebrink do you see some potential issues or drawbacks with this method?

Regards,

@FabianGosebrink
Copy link
Collaborator

No, this is another method and should be fine imho :)

@philippemarcelino
Copy link

No, this is another method and should be fine imho :)

Thx for the quick answer!

@FabianGosebrink
Copy link
Collaborator

You are very welcome and thanks for using our lib :)

@cghislai
Copy link

Its also worth mentionning that the AutoLoginPartialRoutesGuard guard may pass BEFORE checkAuth() is actually called, probably when a valid token is found in storage.

In my case, i wanted to fetch a resource from a remote server once the user is authenticated but before my protected routes could be displayed. I was relying on another guard waiting on a value to be emitted by an observable, but that value was only fetched once my checkAuth call had completed. It appeared the angular app was hanging.

A solution was to call isAuthenticated() in my service constructor, and fetch and emit my resources from there if the user was already authenticated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

10 participants