search

IdentityModel / oidc-client-js

OpenID Connect (OIDC) and OAuth2 protocol support for browser-based JavaScript applications
Apache License 2.0
2.43k stars 842 forks source link

Error status handling for OIDC session management #1355

Open divid3byzero opened 3 years ago

divid3byzero commented 3 years ago

I am using oidc-client-js in an enterprise SSO and SLO environment. We have implemented SLO using the OIDC session management process as defined here: https://openid.net/specs/openid-connect-session-1_0.html by providing the session state and the required OP iFrame. This is the part that works, the statuses "changed" and "unchanged" are propagated as needed.

The problem is that in case of the "error" status the whole message propagation mechanism stops. Even though the specification does not explicitly define the behaviour that should result from the "error" state I would have expceted that this library would provide an event similar to the available userSessionChanged or other events so that the client can react to this error.

As the cases that lead to the "error" state are things like invalid client ID, invalid session state in the sense of e.g. being null or the origin being wrong which are all very security relevant (they basically imply that e.g. the source has been manipulated somehow, the session state generation mechanism on the OPs side does not work etc.), I would have expcted some way to handle this "error" state.

If I have misunderstood something or have overseen something I would be very happy if I could get some hints as how to I can achieve the described use case.

brockallen commented 3 years ago

I think you have it right -- the library won't know why there's an error and the spec doesn't help with knowing if you should keep polling or not, so this library stops the polling. IIRC there's an API to start/stop, so you might be able to reach thru the UserManager and find the object you need to re-start the polling.

divid3byzero commented 3 years ago

Actually, my problem is not that I can't start or restart the polling. There is a configuration parameter to continue polling in the error case (which, by the way, is not documented, I found the parameter "by accident" as I was searching through the code to find a solution for my issue). My problem is that, in case of an error, I want to be notified so I can take further action such as logging the user out.

The issue here is that in case of an error, and as I have already stated the cause for an error is very likely either a security breach or a severe programming error on the OP side, I would like to be able to take further action such as logging out the user. Of course, ~I'll probably~ I might be able to set up an additional check to probe the current users session state by checking the UserManager regularily in an according interval. But this seems unnecessary as there is already an event mechanism in place and this error case should really be handled by that mechanism in the form that the client gets notified and can handle it as seen fit.

If you want I can make a suggestion via pull request if you accept contrubutions (I haven't checked on that to be honest).

brockallen commented 3 years ago

I would like to be able to take further action such as logging out the user.

Locally or also at the OP?

In either case, that sounds dangerous. If you search the archives there are a few issues where people are reporting that "error" is returned for completely unrelated reasons. The main one I can think of is a browser plug in that was hammering all iframes with postMessage, which produced "error".

divid3byzero commented 3 years ago

Locally or also at the OP?

Well, in my case probably at both.

I understand what you mean by dangerous. That is why I suggest that the library could provide an event so the client can decide what needs to be done. This "error status event listening" could additionally be "guarded" by a setting that is set to false per default so that in the default case the session management error status results in the current behaviour but there is also a possibility to activate an error status event so that clients can react to it if they see it fit. I think that it is not possible to say that all clients need or do not need to be able to react to the error status - there should be a possiblity to switch this behaviour on and off.

brockallen commented 3 years ago

Ok, so you're looking for an error event on error. Got it.

divid3byzero commented 3 years ago

Yes, that is exactly what I am looking for. And like I said, I can implement a suggestion and send a pull requerst if you want.

brockallen commented 3 years ago

You can send a PR, but my time is strapped to even review.

divid3byzero commented 3 years ago

Ok, I will implement something and commit via pull request. I am sure that others will also profit from this - this is something that everyone using OIDC session management should potentially need even though it is not explicitly defined by the specification.