aurelia-contrib / aurelia-open-id-connect

An aurelia adapter for the IdentityModel/oidc-client-js
https://zamboni-app.azurewebsites.net
MIT License
54 stars 18 forks source link

Router error after redirect #47

Closed jonathaneckman closed 5 years ago

jonathaneckman commented 6 years ago

I've been tracing this error all week and havent made much progress so I'm hoping you'll see something I dont. After a login redirect, the following happens:

  1. Auth server redirects back to /signin-oidc as expected.
  2. My root component begins to load (app.ts)
  3. OpenIdConnectNavigationStrategies.signInRedirectCallback navigation strategy begins.
  4. Within the aurelia router, something explodes. I believe it is on this line in evaluateNavigationStrategy. I see that instruction.config.moduleId is undefined.
  5. The error below is thrown.
  6. The page refreshes to redirect route specified in OpenIdConnectConfiguration.loginRedirectRoute.
ERROR [app-router] TypeError: Cannot read property 'trim' of undefined
ERROR [app-router] Router navigation failed, and no previous location or fallbackRoute could be restored.

So even though an error is thrown, things seem to be working as expected. Is this a known issue or is this a problem on my end?

I have attempted to clone your plugin to troubleshoot further but get the following error whenever I npm link to this project. This is new for me so I may not be doing it right. I am going to keep trying so if you have any insight into how you run the plugin locally to test within a consuming app I'd appreciate it. I think I've guessed the steps to build, run, and test but if you could document those as well it would be a huge help.

Error: Can't figure out a normalized module name for ./open-id-connect-user-block, please call PLATFORM.moduleName() somewhere to help.
    at Compilation.compilation.plugin.modules (C:\Code\myproject\node_modules\aurelia-webpack-plugin\dist\PreserveModuleNamePlugin.js:44:31)
jonathaneckman commented 6 years ago

I dont believe the npm link issue is caused by anything in this plugin. I reproduced similar issues with aurelia-validation. I submitted an issue to JavaScriptServices.

jonathaneckman commented 6 years ago

@shaunluttin I stepped through this again. The problem is that the routes do not have a module id. I can see that when attempting to load the logInRedirectCallback navigation instruction, the viewPortPlan.config.moduleId is undefined.

Starting in the navigation pipeline: image

Then attempting to load the route: image

Which tries to load a component based on the module id. In aurelia-path the error is thrown because of the call to trim an undefined parameter.

image

I think the fix is simple. A module id needs to be added to the login and logout routes you are configuring in configureRouter.

I am using the route names from your example: signin-oidc and signout-oidc. This error is thrown whether I have configured these routes in my app or not. For example:

{
    route: 'signout-oidc',
    name: 'signout-oidc',
    moduleId: PLATFORM.moduleName('../../routes/signout-oidc/signout-oidc'),
},
jonathaneckman commented 6 years ago

I put together a repro . You can the issue in my configureRouter.

Waiting to see if this is a bug in aurelia-router or if it should be a change here. See this issue there for more info.

shaunluttin commented 6 years ago

You did a ton a research looking in to that and even uploaded screenshots. Thank you for the report!

I have been away from this project for about a month now and am not sure when I will be returning to it.

From quickly reviewing your analysis, it seems that the fix might be simple but also that it might be a bug in the Aurelia router.

I would be happy to hear more about this and will check-in from time to time.

jonathaneckman commented 6 years ago

Sounds good. I have a feature and a blog post waiting on this issue so I'm motivated to help. I plan to showcase this plugin in the post. If the fix is here, are you willing to accept a PR?

shaunluttin commented 6 years ago

Yes. I am definitely willing to accept a PR.

Question: Have you checked out the demo projects? Is the issue there too?

jonathaneckman commented 6 years ago

Yes the issue is in the demo projects.

image

shaunluttin commented 6 years ago

The issue might be related to enhancement https://github.com/shaunluttin/aurelia-open-id-connect/issues/17 that happened with commit https://github.com/shaunluttin/aurelia-open-id-connect/commit/11ea315a8ff741c670e1f33cd332a397aa87efbc.

saspeed commented 6 years ago

I was hoping to find some resolution to this issue this with the new release 2.0.0. But i can still confirm the problem even with the demo project. How is this not affecting more users of the plugin? @jonathaneckman Is there a workaround you can suggest?

khuzemakanore commented 6 years ago

It is affecting me also.

shaunluttin commented 6 years ago

It's good to know that this remains a problem. I'll mark it as a priority; that said, I am not sure when I will have time to address it.

rmja commented 5 years ago

Hi,

I have looked into this and fount that the problem comes from the way redirects are performed for the signin/signout navigation strategies. This issue comes from the fix introduced by https://github.com/aurelia-contrib/aurelia-open-id-connect/commit/e06a1943e6a4688b0d90f5f8a332b8039a29a216 fixing https://github.com/aurelia-contrib/aurelia-open-id-connect/issues/46.

This is the current sequence when signing in:

  1. signin-oidc route is hit from the idp.
  2. window.location is assigned in the navigationStrategy callback registered for the route. No other fields are assigned on the instruction config which is required by the aurelia router.
  3. the router pipeline continues after the navigationStrategy callback has completed, eventually failing with the 'trip' TypeError because the callback did not setup a valid configuration for the router to complete.
  4. The entire application is reloaded because of the assigned window.location. This time the assigned redirectRoute is loaded by the router and not the signin-oidc route.

Here is the desired sequence when signing in:

  1. signin-oidc route is hit from the idp.
  2. instruction.config.redirect is assigned in the navigationStrategy callback which is a valid configuration for the aurelia router. 2.1. I have been unable to reproduce #46, but if it is still an issue, rewrite the history using something like instruction.router.history.navigate(redirectRoute, { replace: true });
  3. the router pipeline continues with a configured redirect, and the router perferms the redirect completing the login.

cc @shaunluttin

shaunluttin commented 5 years ago

We might have resolved this (at least partially) with v2.0.2.

shaunluttin commented 5 years ago

The fix is installed here: https://github.com/aurelia-contrib/aurelia-open-id-connect-demos.

saspeed commented 5 years ago

Awesome, Thanks! I'll verify next week

rusch1981 commented 5 years ago

I believe there is a issue related the fixed installed (v2.0.2).

Running version 2.0.2 I am received the error:
image

After debugging into aurelia-router.js, specifically the _buildNavigationPlan function, I noticed that when returning from identity server the parameter "instruction" (of type NavigationInstruction) was coming through with a "config.redirect" property that contains a value. The value is the url back to the last navigated location of the client application. When _router.generate is called you get an error, because the "newinstruction.config.name" is undefined.

image

When I remove the "config.redirect" property during debug, I recieved the trim error noted in the screen shot provided by @jonathaneckman.

I set the version back to 2.0.1 and I am no longer seeing errors. I hope that is helpful.

arnederuwe commented 5 years ago

Hi, I will be closing up stale issues by the end of the week.

Are you still experiencing this issue? Feel free to close this yourself. If you are still experiencing this issue, let me know as well, thanks!