Call /logout endpoint when logging out

[cblakeley]

Related issue: nodeSolidServer/oidc-auth-manager#20

[coveralls]

Coverage increased (+0.1%) to 85.153% when pulling abde4b4 on OpenLinkSoftware:openlink_pull_req into ab12fa7 on solid:master.

[RubenVerborgh]

I'd like to understand why the /logout is needed in the first place. Also, this cannot be a GET.

[RubenVerborgh]

This should move up, and there should only be one catch block.

(Also, unsure why we're using the then/catch syntax here instead of async/await.)

[RubenVerborgh]

Also, if this is a state-changing operation, this cannot be a GET!

[smalinin]

@RubenVerborgh What must be used instead of GET ? The Solid-server has handler only for GET request

  router.get('/logout', LogoutRequest.handle)

[RubenVerborgh]

Oh, that's a bug in NSS: nodeSolidServer/node-solid-server#744

[smalinin]

@RubenVerborgh I have fixed code GET was replaced with POST and call was moved up.

[RubenVerborgh]

So I've been through this and tested it, but it doesn't work.

While this code makes a call to /logout, but that doesn't do the trick. What is actually needed is a proper logout along the lines of nodeSolidServer/oidc-rp@a78420b, but that commit hasn't made it to the final version yet.

To summarize: some of our dependencies need updates first in order for this to work. We'll have to wait on nodeSolidServer/oidc-auth-manager#20 for a proper fix.

[smalinin]

@RubenVerborgh I wrote already in => https://github.com/solid/node-solid-server/pull/701/files/20987f6ef23940805da58b611b4f64f95e897298#diff-237045f786d4918cf906b96a1c163eb9
that also the next fix is required for server side
file ./lib/create-app.js

      // without permission by including the credentials set by the Solid server.
      const origin = req.headers.origin
      const userId = req.session.userId
-      if (!argv.host.allowsSessionFor(userId, origin)) {
+      if (req.path !== '/logout' && req.path !== '/goodbye' && !argv.host.allowsSessionFor(userId, origin)) {

The logout will do nothing on server side without fix above, because it will be blocked by server.
Did you have such fix on server side, when you tested the fix?

[RubenVerborgh]

We don't; so we first need this to land on the server before we continue on the client side.

But regardless, it does not seem like it's up to solid-auth-client to make this HTTP request, but rather to the OIDC library (which makes this request, but doesn't include cookies).

[smalinin]

@RubenVerborgh Ok,
Could you also recheck module dependency in Node-solid-server project ?
I have installed latest version of NSS from master branch( and executed npm install) and checked version of oidc-rp package in ~/node_modules/@trust/oidc-rp/
The version is => "@trust/oidc-rp@0.4.3", .
Is it Ok ?
I checked git https://github.com/solid/oidc-rp and I found version "version": "0.6.0" !!!

[RubenVerborgh]

It's not okay, our OIDC versioning is problematic currently. This is because our oidc-module is a fork. @justinwb is looking into this.

[dmitrizagidulin]

Like I mentioned in Gitter, we have a /different/ fix server side. And the logout API has changed to ‘logoutRequest()’ on the rp side.

[RubenVerborgh]

@dmitrizagidulin Main problem is that the logoutRequest API isn't present in a released version. Can you coordinate with @justinwb?

[dmitrizagidulin]

Yep, in the process of doing so.

[smalinin]

@RubenVerborgh This fix isn't required, I rechecked all again. The problem was with:

1. our fix for logout() method in oidc-rp package
2. with current implementstion of logout() in oidc-rp, it works wrong:
   a) it call GET /logout after clearSession(), so server reject unauthorized /logout call
   b) GET /logout call doesn't have option credentials: 'include', so browser didn't clear session cookie.
   I will prepare and send fix for oidc-rp package later.

[RubenVerborgh]

@smalinin Indeed, that's what I also concluded in #46 (comment).

I unfortunately went a bit too fast and have prepared nodeSolidServer/oidc-rp#11, but feel free to create your own. Won't merge until I hear back from you.