You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Heh, so this is the issue that was being addressed in the closed issue #675. (Though I totally get that 675 was named in an overly-specific nondescript manner).
So here's the story. Server-side support for this has been implemented in nodeSolidServer/oidc-op#7 (though that PR still needs some tests, that's been the main holdup). Once that's merged, it'll need to be integrated into node-solid-server.
On the client-side, this has been implemented in PR nodeSolidServer/oidc-rp#5. That changes how an auth client needs to use the logout call, etc.
Yeah, so that's what I mean that it was badly named :) #675 is about fixing the POST handling of the /logout endpoint (and removing the existing GET /logout route in webid-oidc.js). And the thing that was blocking /that/ was post-logout redirect handling.
Anyways, I figured I'd give an overview of where the story stands right now.
Ah, wait, though. Now I remember. So there's a couple of reasons why the /logout endpoint might still need to be a GET.
One, it may be useful to retain the ability for a user to just visit their pod's /logout endpoint via just typing it in to the url bar.
I suppose we can just require them to press a Logout button somewhere (which submits a form POST). When the user presses the logout button, a couple things need to happen. They need to be prompted with a choice screen asking if they also want to log out of the pod (in addition to logging out of the RP app). Their browser cookies (which are http only, not JS) need to be cleared. And then they need to be redirected to the post_logout_redirect_uri.
So, basically, one of the ways to get around the fact that logging out changes state, is that a GET to /logout just gets you a confirmation screen (idempotent). Which then POSTS to the /logout endpoint if the user confirms.
As already stated, GET is appropriate if the request is idempotent (isn't state changing). Otherwise it should be POST, despite under specification in the OIDC docs.
"a method is idempotent if repeated invocations have the same effect as one invocation" in my understanding of English, means it doesn't change state i.e., if you repeat invocations that produce the same effect, said effect hasn't been changed or altered :)
"a method is idempotent if repeated invocations have the same effect as one invocation" in my understanding of English, means it doesn't change state
No, that's not correct. They do change state on the first successful invocation, but any subsequent invocations do not incur an additional state change. E.g., DELETE is idempotent, but does change state.
I added a POST route because logging out is a state change, but I'm keeping the GET around for now (since removing that requires a major version change).
If other OIDC implementations are using GET (which is the case) then removing it could make the solid implementation problematic. That said, we have a generic client which could be used to test this claim against a solid server. Basically, we end up in the same place i.e., when can we test a solid-server implementation of OIDC that has all the recent fixes applied.
Note, we have Virtuoso and our OSDB tools in place as generic OIDC clients, distinct from what we've done with our solid-server fork.
Activity
RubenVerborgh commentedon Aug 9, 2018
We can already implement a parallel route with POST for 4.1, and then drop the GET route for 5.0 (semver.major).
dmitrizagidulin commentedon Aug 9, 2018
Heh, so this is the issue that was being addressed in the closed issue #675. (Though I totally get that 675 was named in an overly-specific nondescript manner).
So here's the story. Server-side support for this has been implemented in nodeSolidServer/oidc-op#7 (though that PR still needs some tests, that's been the main holdup). Once that's merged, it'll need to be integrated into node-solid-server.
On the client-side, this has been implemented in PR nodeSolidServer/oidc-rp#5. That changes how an auth client needs to use the logout call, etc.
RubenVerborgh commentedon Aug 9, 2018
#675 about the post-logout redirection URL, right?
#744 is about using HTTP
POSTto log out instead ofGET.dmitrizagidulin commentedon Aug 9, 2018
Yeah, so that's what I mean that it was badly named :)
#675 is about fixing the POST handling of the
/logoutendpoint (and removing the existingGET /logoutroute in webid-oidc.js). And the thing that was blocking /that/ was post-logout redirect handling.Anyways, I figured I'd give an overview of where the story stands right now.
RubenVerborgh commentedon Aug 9, 2018
Makes sense, thanks.
dmitrizagidulin commentedon Aug 9, 2018
Ah, wait, though. Now I remember. So there's a couple of reasons why the
/logoutendpoint might still need to be a GET.One, it may be useful to retain the ability for a user to just visit their pod's
/logoutendpoint via just typing it in to the url bar.I suppose we can just require them to press a Logout button somewhere (which submits a form POST). When the user presses the logout button, a couple things need to happen. They need to be prompted with a choice screen asking if they also want to log out of the pod (in addition to logging out of the RP app). Their browser cookies (which are http only, not JS) need to be cleared. And then they need to be redirected to the post_logout_redirect_uri.
So, basically, one of the ways to get around the fact that logging out changes state, is that a GET to /logout just gets you a confirmation screen (idempotent). Which then POSTS to the /logout endpoint if the user confirms.
RubenVerborgh commentedon Aug 9, 2018
Indeed. As long as the GET (or any GET, for that matter) doesn't trigger a state change, it's fine.
Add POST route for /logout.
RubenVerborgh commentedon Aug 11, 2018
Unfortunately, I see lots of OIDC examples where logout is a GET request, so maybe this is just how it's done.
The OIDC spec itself does not mention anything though: http://openid.net/specs/openid-connect-session-1_0.html#RPLogout
kidehen commentedon Aug 15, 2018
@RubenVerborgh ,
As already stated, GET is appropriate if the request is idempotent (isn't state changing). Otherwise it should be POST, despite under specification in the OIDC docs.
/cc @cblakeley @smalinin
RubenVerborgh commentedon Aug 15, 2018
That's incorrect.
GET/HEAD is safe and idempotent; PUT/DELETE is not safe but idempotent; POST is neither.
Logging out is a state change, so GET isn't appropriate.
kidehen commentedon Aug 15, 2018
@RubenVerborgh ,
"a method is idempotent if repeated invocations have the same effect as one invocation" in my understanding of English, means it doesn't change state i.e., if you repeat invocations that produce the same effect, said effect hasn't been changed or altered :)
BTW -- what is the conclusion to this issue?
RubenVerborgh commentedon Aug 15, 2018
No, that's not correct. They do change state on the first successful invocation, but any subsequent invocations do not incur an additional state change. E.g.,
DELETEis idempotent, but does change state.See the definition of safe and idempotent methods in the HTTP spec.
I added a POST route because logging out is a state change, but I'm keeping the GET around for now (since removing that requires a major version change).
melvincarvalho commentedon Aug 15, 2018
How about we keep GET around until the openlink work is merged. Then think about deprecating it, and also updating the patch?
kidehen commentedon Aug 15, 2018
@RubenVerborgh,
With POST and GET in place, what happens next? Is this issue closed or there is something else?
RubenVerborgh commentedon Aug 15, 2018
Deprecation or removal of GET for 5.0.0 (if that turns out to be compatible with the rest of the OIDC world).
kidehen commentedon Aug 15, 2018
@RubenVerborgh,
If other OIDC implementations are using GET (which is the case) then removing it could make the solid implementation problematic. That said, we have a generic client which could be used to test this claim against a solid server. Basically, we end up in the same place i.e., when can we test a solid-server implementation of OIDC that has all the recent fixes applied.
Note, we have Virtuoso and our OSDB tools in place as generic OIDC clients, distinct from what we've done with our solid-server fork.
/cc @cblakeley @smalinin @dmitrizagidulin
RubenVerborgh commentedon Aug 15, 2018
Indeed, so we should tread carefully here.