feat(login): redirect request route

gerbendekker commented 6 years ago

Closes #49 by adding request route state

shaunluttin commented 6 years ago

That great. I will do some testing of this today. Thank you for the PR!

shaunluttin commented 6 years ago

Hi @gerbendekker .

I took a look at this today and explored some alternative approaches that we might want to take. The ideas are only partially developed for now, but I will put them here anyway. The quotes are from the original issue https://github.com/aurelia-contrib/aurelia-open-id-connect/issues/49

User is not logged in. Requests app.com/person/1. App redirects them to login.

If person/1 is a restricted route, the redirection to login will happen automatically in \src\open-id-connect-authorize-step.ts ln 32. There, we could include a ?loginRedirectRoute=... query string when we automatically redirect to the login page. Here is some prototype code.

src\open-id-connect-authorize-step.ts ln 32 (approx)

// capture the route to which the user was originally navigating (e.g. person/1)
var loginRedirectRoute = encodeURIComponent(
    navigationInstruction.fragment + "?" + navigationInstruction.queryString);

// store that route in a query string when we redirect to the unauthorizedRedirectRoute
const redirect = new Redirect(
    this.configuration.unauthorizedRedirectRoute + 
    "?loginRedirectRoute=" + loginRedirectRoute);

After being redirected to the login page (which for now we'll assume is the configured unauthorizedRedirectRoute), the user will click Login, and we will flow the loginRedirectRoute from the query string to the login() method. Here is some prototype code for that.

src\open-id-connect.ts ln 28 (approx).

const loginRedirectRoute = decodeURIComponent(/*loginRedirectRouteFromQueryString*/);
const args = { redirect_url: loginRedirectRoute };
await this.userManager.signinRedirect(args);

Notice that the signinRedirect(args) can have a redirect_url key. The userManager actually flows those args to the OidcClient.createSigninRequest, which in turn overrides the settings value. See https://github.com/IdentityModel/oidc-client-js/blob/dev/src/OidcClient.js#L42 for details. The upshot of this in-built behavior is that we do not have to manage the details of the post-login redirection.

The[y] login on the id server. After login they're returned to app.com/person/1

Since we're using the built-in override mechanism of the user manager, that redirect will happen automatically, because we have overridden the redirect_url. It will "just work" if I am not mistaken.

Does this half-baked idea make any sense to you? What are your thoughts on this partial redesign of what you have already done?

P.S. @mttmccb please let us know more details of your requirements, especially if we seen to have gotten them wrong.

mttmccb commented 6 years ago

Looks good, I neglected to mention I did get it working with a hack where I pass the url requested in the state. But it sounds like there is an easier way to handle that.

I've since reverted that code as it was buggy and didn't work as I expected. It was based on what I found on in this issue.

shaunluttin commented 6 years ago

@mttmccb I'm wondering about the ideal flow:

User goes to app.com/private/1
App redirects to app.com/login/?loginRedirectRoute=private/1 (assuming /login is the configured unauthorizedRedirectRoute).
User sees a message that private/1 is secure and sees a button to login.
User clicks login.
App redirects to the identity server with ?redirect_url=app.com/private/1&other=other&another=...
User logs in at the identity server.
Identity server redirects to app.com/private/1

How does that look to you?

mttmccb commented 6 years ago

@shaunluttin Yes looks good, within the app you could of course set up a redirect so they don't have to click. I have something like that in the app I'm working on, it displays a message and then redirects them to the identity server.

shaunluttin commented 6 years ago

Let me see if I understand correctly. You're suggesting that it goes directly from step (1) to step (5). Is that right?

mttmccb commented 6 years ago

They seems like an unnecessary steps but if you configure the unauthorizedRedirectRoute the developer would choose right? Or is /login mapped to oidc.login()?

The other scenario is they are requesting access to the app.com and I would expect a redirect right to the ID server in that case.

shaunluttin commented 6 years ago

Jumping from step (1) to step (5) seems like it would present a problem. The end-user could no longer choose the identity provider to use, when there are multiple providers from which to choose. Also, if I am not mistaken, it looks like the industry standard flow for a private page is to cue the user to login before redirecting the user to the identity server. (e.g. https://gitter.im/shaunluttin does that). It would be worth seeing some examples of the jump from step (1) to (5). Twitter, for instance, jumps us right to the login page (https://twitter.com/settings/account). That said, they are not redirecting to a separate identity server.

shaunluttin commented 6 years ago

if you configure the unauthorizedRedirectRoute the developer would choose right?

Good point. I like the idea of letting the developer choose. (I'm off to go hip hop dancing... chat again soon. Thank you for the feedback @mttmccb).

gerbendekker commented 6 years ago

Does this half-baked idea make any sense to you?

Yes it does, we sure need some additions to solve this one. Sound great so far! Do we have to take into account possible additional user roles? I think this can be done separately from this issue. Currently we only have is authenticated.

gerbendekker commented 6 years ago

The longer I'm thinking about this... I can imagine of the following scenarios:

The described scenario by @shaunluttin app.com/private/1 results in unauthorizedRedirectRoute
The one partial covered by the pull request. Anonymous user navigate to app.com/orders and gets readonly list of orders
- switch: user wants to delete an order (we stick on route app.com/orders)
  - case: user hits the delete button
  - show unauthorizedRedirectRoute (developer choice)
  - original route retrieved from navigationInstruction is stored in query string
  - user hits login and navigate to login through OpenIdConnect.login()
  - original route is retrieved from decodeURIComponent
  - case: user hits the delete button
  - navigate to login through OpenIdConnect.login() (developer choice)
  - original route is retrieved from this.router.currentInstruction
  - case: user knows he need to login and will hit login button on the header
  - navigate to login through OpenIdConnect.login()
  - original route is retrieved from this.router.currentInstruction

shaunluttin commented 6 years ago

@gerbendekker I'd like to take the time to do this feature correctly and work with you on it. There are two other side projects that I need to finish first. That will take me about one or two weeks, at which point I'd love to re-engage with aurelia-open-id-connect. How does that sound to you?

gerbendekker commented 6 years ago

@shaunluttin Not specific to this pull request but we are all in the same position, I to have a full time job. Don't know I will have the time in a few weeks, planning effort into contributions won't work (unless my boss finds its important). That's why, in my humble opinion, its important to share whats on your mind:

directions,;
whats missing;
where we need to work on.

Create issues for it because maybe somebody else in the community has created something in a project or can think along.

shaunluttin commented 6 years ago

@gerbendekker That sounds good. We'll work asynchronously. :-)

shaunluttin commented 6 years ago

I've been considering the merit of writing some contract tests for the library before making additional changes. That would help to ensure that we do not break existing library consumers.

josundt commented 6 years ago

Any progress here? It's been some time, and I really need to take the user to the page initially requested after completing the login flow.

I believe this is the application behavior expected by users, but I can't make this work until the AuthorizeStep in the plugin is improved somehow - the minimum requirement is to pass the unauthorized URL as route parameter (querystring) to the "unauthorized" route.

You could make this a opt-in feature of the plugin; configurable through a new optional plugin setting. Then you would avoid breaking changes.

Hoping for a fix and a release as soon as possible!

shaunluttin commented 6 years ago

There hasn't been any progress on this for quite some time, because I have become busy on other projects and technology stacks. The best I can suggest is to fork the repository and to merge the changes that you require into that fork.

shaunluttin commented 5 years ago

Thank you for this PR. I am going to start taking a lenient approach with the pull-requests, to prevent the project from becoming dead. I will give it a light review and merge if everything looks reasonable. Then before bumping the version and publishing to NPM, I will take a closer look at the changes. As you might have noticed, I am a reluctant open-source maintainer. Thank you again for the PR. @gerbendekker @mttmccb @josundt

aurelia-contrib / aurelia-open-id-connect

feat(login): redirect request route #51