WebSocket NEXT: automatically close connection when OIDC extension provides SecurityIdentity and token expires

[michalvavrik]

• Closes: WebSockets Next: close the connection if the security identity has expired #40536

[github-actions]

🙈 The PR is closed and the preview is expired.

[sberyozkin]

Thanks @michalvavrik. Can users have a chance to react to this closure somehow, have the error handler called etc ?

[michalvavrik]

Thanks @michalvavrik. Can users have a chance to react to this closure somehow, have the error handler called etc ?

Connection is getting closed, you shouldn't be able to stop it. What else can be done?

If we only fired on error, there are 4 strategies (close, log, call, do nothing, custom OnError method) and only the first one is suitable. Please take this with big reservation because I am no expert but IMO: anything but closing is not thread-safe because we cannot ensure that the identity is updated before next on message is called and we don't have way to stop everything till re-authentication happens (like token refresh).

I have added assertion that OnClose is called, hope it helps.

[sberyozkin]

@michalvavrik I'm thinking what the user experience will be like. Lets say we authenticate with Google into a Quarkus endpoint which returns HTML page, with an option to start a chatbot. The user clicks on it and a web socket session starts, I guess in this case we have the larger HTML page with text like Hello, Alice and then a smaller embedded WS window where a chat bot conversation is ongoing.

Lets say the session has expired. If now Alice presses on anything in the HTML page like a link to another HTML page, Quarkus may auto-refresh the session or redirect Alice to Google to re-authenticate.
But what if, after the session has expired, Alice types something in the WS channel, what will she see in the ChatBot window if the connection is closed ?

[mkouba]

I have added assertion that OnClose is called, hope it helps.

+1

Lets say the session has expired. If now Alice presses on anything in the HTML page like a link to another HTML page, Quarkus may auto-refresh the session or redirect Alice to Google to re-authenticate.
But what if, after the session has expired, Alice types something in the WS channel, what will she see in the ChatBot window if the connection is closed ?

It's up to the client I would say. I.e. if the connection is closed then the client (e.g. some JS code) can decide to redirect the user.

[sberyozkin]

Just curious, ideally, the user should realize that the ChatBot error related to the closed connection means it is time to refresh the surrounding HTML page

[mkouba]

Loogs good!

[michalvavrik]

@michalvavrik I'm thinking what the user experience will be like. Lets say we authenticate with Google into a Quarkus endpoint which returns HTML page, with an option to start a chatbot. The user clicks on it and a web socket session starts, I guess in this case we have the larger HTML page with text like Hello, Alice and then a smaller embedded WS window where a chat bot conversation is ongoing.

Lets say the session has expired. If now Alice presses on anything in the HTML page like a link to another HTML page, Quarkus may auto-refresh the session or redirect Alice to Google to re-authenticate. But what if, after the session has expired, Alice types something in the WS channel, what will she see in the ChatBot window if the connection is closed ?

I agree with Martin's comments - client has to handle this.

I have one idea, but it would need to be a separate issue and I don't know if it makes sense - for a code flow we know that token is going to expire and if refreshing token is possible asynchronously via WebClient POST request, we could do that when the expiration time is getting closer. However at one time right before we synchronize there would be old token (not yet expired) used by the identity and a new issued token.

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 514c426.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

⚠️ There are other workflow runs running, you probably need to wait for their status before merging.

You can consult the Develocity build scans.

[sberyozkin]

It's up to the client I would say. I.e. if the connection is closed then the client (e.g. some JS code) can decide to redirect the user.

Sure, this is what I was curious about, as long as the script can intercept this error and decide what to do, it is all good

[quarkus-bot]

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 514c426.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

[sberyozkin]

Hi @michalvavrik

for a code flow we know that token is going to expire and if refreshing token is possible asynchronously via WebClient POST request, we could do that when the expiration time is getting closer. However at one time right before we synchronize there would be old token (not yet expired) used by the identity and a new issued token.

This is Ok I guess, the WS session will still use valid identity until it expires, even if outside of the WS channel a new token is available... Let's discuss it further when you'd like

[sberyozkin]

Proposing a backport to 3.11 as a hardening fix, which ensures the WS session can't go forever if the identity has expired. But, @gsmet, please remove the backport label if you have some concerns about backporting it

[gsmet]

I tried to backport that one but we would need to also backport #40655 for the tests to pass.

I will let @mkouba decide what's best: either backport both or none (or adjust the tests).

[mkouba]

I tried to backport that one but we would need to also backport #40655 for the tests to pass.

I will let @mkouba decide what's best: either backport both or none (or adjust the tests).

I would backport both... if possible. Thanks.

[gsmet]

I would backport both... if possible. Thanks.

@mkouba done with 3.11.2