Fix post logout redirect, add config for default

[balazsorban44]

By submitting a PR to this repository, you agree to the terms within the Auth0 Code of Conduct. Please see the contributing guidelines for how to create and submit a high-quality PR for this repo.

Description

When invoking the default /logoutendpoint, we will try to attempt to get the returnURL from req.query.returnTo. In addition, I added the possibility to provide post_logout_redirect_uri in the config of auth(), like so:

express()
//...
.use(auth({
    baseURL: "http://localhost:8000",
    post_logout_redirect_uri: "http://localhost:8000",
    required: false,
    idpLogout: true
  }))
// ...

@joshcanhelp I also did a change which I ask you to give a comment on. I moved the session destroy part under the client.endSessionUrl() call, because req.openid.tokens becomes undefined after destroy. After this, my IdP redirects correctly as referred to in the issue referenced under.

References

This issue is an improvement after the discussion in #35.

Testing

As of now I only tested it locally, but everything seems to work fine. If requested and/or the way I implemented it is accepted, I can try to create some tests in logout.tests.js

• This change adds test coverage for new/changed/fixed functionality

Checklist

Again, if my changes are accepted, I can document the changes.

• I have added documentation for new/changed functionality in this PR or in auth0.com/docs
• All active GitHub checks for tests, formatting, and security are passing
• The correct base branch is being used, if not master

[balazsorban44]

ping @joshcanhelp 🙂

[joshcanhelp]

@balazsorban44 - Appreciate your patience here. This is really close, just a few small changes and we'll get this merged in. Also, would be great to get a few tests written to make sure that the URL is coming through in all the cases.

[balazsorban44]

Moved some parts around a bit in the tests, hopefully it is OK with you. Moved the before call into a setup function, that takes some params to slightly change the result (able to modify auth params per test now, with a single setup).

Used this great article by Kent.C.Dodds: Avoid Nesting when you're Testing

Also moved repetitive strings to a single place (mockData, authOParsedUrl), so typos and forgetting to replace strings (eg.: version number in pathname) at all the places are out. (That part is only my own preference for the above mentioned reasons, might have been a bit fast with that, can change back if you wish)

[joshcanhelp]

RE: the test changes ... while I'm not under any delusions that the current way is the right way 😄, I'm a little worried about changing the whole testing paradigm in a single file. It's also not clear what tests were added for this functionality. Maybe we can address that approach in a different PR?

[joshcanhelp]

@balazsorban44 - Are you able to wrap this one up in the near future? We're going to put out a release next week and it would be great if this was part of that. My main issue in merging this is the big changes in the logout tests. Would like to see primarily adding there, not a refactor.

If you've moved on from the library, let me know and we'll handle it on our end.

Thank you!

[balazsorban44]

Hi, I am back after a long holiday. So to sum up, what is missing so this can be merged? (apart from fixing the conflicts).

[joshcanhelp]

@balazsorban44 - Thanks for checking in. Main thing for me is to just see added tests for this functionality rather than a refactor of how the existing tests work. Also the comment about redirectUriPath, that makes more sense as a path since it will not be external.

[balazsorban44]

OK, will fix next week 😉

[balazsorban44]

Fixed it, but because of the merged in master, some tests are failing.

Both postRedirectUri and logout query string returnTo are relative paths now, which are concatenated with the baseURL.

[joshcanhelp]

Thanks for sticking with this @balazsorban44! I fixed those tests in master so you should be able to rebase and pass here.

Just to be clear, this assumes that any customization it receives is (parameter, config key) is a relative path? Makes sense that it would be consistent across the board.

The one thing we probably want to change here is postLogoutRedirectUri -> postLogoutRedirectPath so it's consistent with the other internal path configurations.

Otherwise, looks great!

[balazsorban44]

We are close! (Sorry if it is a bigger task for such a small PR than you expected 😅. If you only wish to be done with this for now, and rather do this discussion later in an issue for example, just give me a word, and I'll just do the required changes so you will be able to include it in the release.)

I know it is a bit back-and-forth, but I just looked up some docs before I made the name change:

1. OpenID Connect Session Management says:

OPTIONAL. URL to which the RP is requesting that the End-User's User Agent be redirected after a logout has been performed. The value MUST have been previously registered with the OP, either using the post_logout_redirect_uris Registration parameter or via another mechanism. If supplied, the OP SHOULD honor this request following the logout.

2. IdentityServer End Session Endpoint says:

If a valid id_token_hint is passed, then the client may also send a post_logout_redirect_uri parameter. This can be used to allow the user to redirect back to the client after sign-out. The value must match one of the client’s pre-configured PostLogoutRedirectUris.

3. ASP.NET Core says:

The uri where the user agent will be returned to after application is signed out from the identity provider. The redirect will happen after the SignedOutCallbackPath is invoked.
This URI can be out of the application's domain. By default it points to the root.

Auth0 mentions postLogoutRedirectUri in some form here as well.

Maybe none of these are relevant in our case, but I just wanted to clarify if you want to go that way, and really call it postLogoutRedirectPath, because although there is nothing wrong with it in my opinion, but it does not seem to appear in the OIDC spec in that form. If you look at 1. and 2., they do say that you MUST register this URI beforehand. Using a relative path kind of does that, I guess...?

So I don't say I wouldn't change it to postLogoutRedirectPath, but I first would like to hear your opinion about this.

[joshcanhelp]

Appreciate the research and thought on this @balazsorban44. And no worries about this becoming a long discussion. I would much rather talk this through completely and make a good choice than have to deal with pain points later.

I typed out a bunch of stuff and ended up deleting it because the spec does not mention anything about the base domain of those post logout URIs. I was assuming that those needed to be on-site but that's not true according to the spec. As long as it's registered on the OP, then it's fine.

I'm going to run this by my team quickly but I'm now in agreement here about the uri part. Thank you again for starting the discussion!

One thing that would be nice here is to accept a path or a URI and append the base URL if it's a path. Does that sound reasonable?

[balazsorban44]

Yes, supporting both may be the most reasonable decision. :) I will have a look at it.

[balazsorban44]

Hmm. 🤔 Should full URIs be supported for returnTo as well? 🔒

In that case I could also create an optional postLogoutRedirectUris array in the config, that accepts a list of relative paths or URIs, and force params.returnTo and req.query.returnTo to be in that list, making it mandatory to pre-register. I am not 100% sure though how I should handle an error then if it occurs in the logout() function.

UPDATE:
I went ahead, and made those changes, I can revert it if you disagree, but I now do the following:

1. If postLogoutUri is a relative path, it is prepended with baseURL.
2. If postLogoutUri is a full URI, we can safely redirect to it, because it was defined in the backend.
3. If returnTo parameter is a relative path, we proceed as in 1.
4. If returnTo parameter is a URI starting with baseURL, proceed, cause it is safe.
5. If returnTo parameter is a URI with an external domain, only proceed if the URI is explicitly registered in postLogoutRedirectUris. Otherwise throw an error:

new Error(`returnTo (${returnTo}) URI was not registered in config's postLogoutRedirectUris.`);

[joshcanhelp]

Apologies for the delayed response here. Let me know if you have any questions about my feedback here.

[joshcanhelp]

Wrong review type ... comments above.

[balazsorban44]

I reverted my postLogoutRedirectUris changes then. See above why the tests must include '/'.

[joshcanhelp]

Looks and works like a charm. Thanks @balazsorban44!

[joshcanhelp]

@balazsorban44 - I tried to rebase master for you but your branch won't allow pushes. Can you resolve the conflicts here and push? If you're able to rebase the commits to cut down on the number there, that would be helpful as well.

[balazsorban44]

@joshcanhelp Should be OK now, I did not have any merge conflicts though.