Skip to content

Conversation

rsvoboda
Copy link
Member

Tweaks for permission checkers for WebSockets Next

Part of work on QUARKUS-5859 Support permission checkers for WebSockets Next

  • PermissionChecker JavaDoc link
  • Uni usage in PermissionChecker and WS endpoint, OnError with WebSocketConnection

Copy link

quarkus-bot bot commented Apr 21, 2025

Status for workflow Quarkus CI

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

✅ 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.

Warning

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

You can consult the Develocity build scans.

Copy link
Member

@michalvavrik michalvavrik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a review from @mkouba and @sberyozkin.

@OnOpen
String open() {
return "ready";
Uni<String> open() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering io.quarkus.websockets.next.test.security.HttpUpgradePermissionCheckerTest.Checker#canDoReadOnEndpoint doesn't check open, the check is happening on the upgrade, this shouldn't have any relation at all. But that's just FYI, I don't think there is anything wrong about the change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, and what's the actual reason for this change?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rsvoboda Hi Rostislav, can you please reply here to Martin before we merge ? I'm assuming the plan is to increase the test coverage of different open response types

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, to cover different response type. I missed that, thanks for reminder @sberyozkin

Copy link

quarkus-bot bot commented Apr 21, 2025

Status for workflow Quarkus Documentation CI

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

✅ 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.

Copy link

github-actions bot commented Apr 21, 2025

🙈 The PR is closed and the preview is expired.

@sberyozkin
Copy link
Member

Thanks @rsvoboda @michalvavrik,
LGTM, perhaps we should copy those tests to keep testing non-Uni responses and add the ones returning Uni ?

@rsvoboda
Copy link
Member Author

@sberyozkin, about Uni<Boolean> and Uni<String>, I tried to go a non-invasive way, not to drop coverage for non-Uni responses. They are still covered:

  • hasPerm1 in PayloadPermissionCheckerTest returns a boolean, I changed just hasPerm2
  • @OnOpen methods in PayloadPermissionCheckerTest return String
  • Only @OnOpen method for AdminEndpoint in HttpUpgradePermissionCheckerTest returns Uni<String>

@sberyozkin
Copy link
Member

Thanks @rsvoboda, great both Uni and non Uni variations are covered

@sberyozkin sberyozkin merged commit 0617b06 into quarkusio:main Apr 22, 2025
25 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.23 - main milestone Apr 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants