Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WebSockets Next: provide strategies to process unhandled failures #40648

Closed
mkouba opened this issue May 15, 2024 · 9 comments · Fixed by #40655
Closed

WebSockets Next: provide strategies to process unhandled failures #40648

mkouba opened this issue May 15, 2024 · 9 comments · Fixed by #40655
Labels
Milestone

Comments

@mkouba
Copy link
Contributor

mkouba commented May 15, 2024

Description

Right now, we log an error message when a unhandled failure occurs (unless it's a "WebSocket closed" error in which case we only log a debug message).

It seems reasonable to provide several strategies instead:

  1. Close the connection (default)
  2. Log an error message
  3. NOOP
@sberyozkin
Copy link
Member

It would nice to have an option to redirect a user to a configured page once the connection is closed

@mkouba
Copy link
Contributor Author

mkouba commented May 15, 2024

It would nice to have an option to redirect a user to a configured page once the connection is closed

I don't think that's possible. There's no HTTP response once the upgrade is finished. We could use a specific close reason though.

@mkouba
Copy link
Contributor Author

mkouba commented May 15, 2024

It would nice to have an option to redirect a user to a configured page once the connection is closed

I don't think that's possible. There's no HTTP response once the upgrade is finished. We could use a specific close reason though.

I mean not possible after the upgrade but it should be possible if we detect a problem during the handshake, e.g. once we implement #40622.

However, this issue is more generic, not specifically related to security.

@sberyozkin
Copy link
Member

I mean not possible after the upgrade but it should be possible if we detect a problem during the handshake, e.g. once we implement #40622.

Sure

@michalvavrik
Copy link
Member

michalvavrik commented May 15, 2024

  1. Close the connection (default)
  2. Log an error message
  3. NOOP

That would be very helpful. But there is a question: Ideally @Authenticated and @RolesAllowed are just user-friendly alternatives to path-matching HTTP Security policies (perms). So if the authenticated policy is performed eagerly and returns 401, we should be able to perform security checks for security annotations applied on the @OnOpen (either by method level or inherited from the class level) and perform it eagerly as well. So we would return 401 and we wouldn't call @OnError as the WS connection has never been established. We authenticate the initial HTTP request, not WS connection.

WDYT?

@michalvavrik
Copy link
Member

BTW I realize this issue is not security specific, it applies for other failures as well, but I hope the question is still valid.

@michalvavrik
Copy link
Member

michalvavrik commented May 15, 2024

I mean not possible after the upgrade but it should be possible if we detect a problem during the handshake, e.g. once we implement #40622.

maybe I should had commented on #40622, apologies.

@mkouba
Copy link
Contributor Author

mkouba commented May 15, 2024

I mean not possible after the upgrade but it should be possible if we detect a problem during the handshake, e.g. once we implement #40622.

maybe I should had commented on #40622, apologies.

No problem. Speaking of "eagerly performed @OnOpen" - I'm afraid that this would complicate the workflow a lot. So I think that we should rather consider the case where security annotations are declared on the endpoint class and applied before the endpoint instance is actually created.

@michalvavrik
Copy link
Member

I mean not possible after the upgrade but it should be possible if we detect a problem during the handshake, e.g. once we implement #40622.

maybe I should had commented on #40622, apologies.

No problem. Speaking of "eagerly performed @OnOpen" - I'm afraid that this would complicate the workflow a lot. So I think that we should rather consider the case where security annotations are declared on the endpoint class and applied before the endpoint instance is actually created.

Sounds acceptable. We can collect feedback from others. Personally I think if it is highlighted in docs it will be useful feature. Thank you

mkouba added a commit to mkouba/quarkus that referenced this issue May 15, 2024
- resolves quarkusio#40648
- also add WebSocketConnection#closeReason() and
  WebSocketClientConnection#closeReason()
mkouba added a commit to mkouba/quarkus that referenced this issue May 15, 2024
- resolves quarkusio#40648
- also add WebSocketConnection#closeReason() and
  WebSocketClientConnection#closeReason()
mkouba added a commit to mkouba/quarkus that referenced this issue May 16, 2024
- resolves quarkusio#40648
- also add WebSocketConnection#closeReason() and
  WebSocketClientConnection#closeReason()
@quarkus-bot quarkus-bot bot added this to the 3.12 - main milestone May 16, 2024
gsmet pushed a commit to gsmet/quarkus that referenced this issue Jun 10, 2024
- resolves quarkusio#40648
- also add WebSocketConnection#closeReason() and
  WebSocketClientConnection#closeReason()
@gsmet gsmet modified the milestones: 3.12 - main, 3.11.2 Jun 10, 2024
holly-cummins pushed a commit to holly-cummins/quarkus that referenced this issue Jul 31, 2024
- resolves quarkusio#40648
- also add WebSocketConnection#closeReason() and
  WebSocketClientConnection#closeReason()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants