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: Security cleanup #43882

Merged
merged 1 commit into from
Oct 16, 2024

Conversation

mkouba
Copy link
Contributor

@mkouba mkouba commented Oct 15, 2024

  • do not force authentication in a dedicted handler so that it's possible to capture the SecurityIdentity before the HTTP upgrade but use the deferred identity instead
  • also change the HttpUpgradeContext to consume Uni instead of SecurityIdentity

- do not force authentication in a dedicted handler so that it's
possible to capture the SecurityIdentity before the HTTP upgrade but use
the deferred identity instead
- also change the HttpUpgradeContext to consume Uni<SecurityIdentity>
instead of SecurityIdentity
@mkouba
Copy link
Contributor Author

mkouba commented Oct 15, 2024

This is a breaking change in the HttpUpgradeContext and should be documented in the migration guide. Although the HttpUpgradeContext is not even mentioned in the docs.

Copy link

quarkus-bot bot commented Oct 15, 2024

Status for workflow Quarkus CI

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

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

You can consult the Develocity build scans.

@michalvavrik
Copy link
Member

This is a breaking change in the HttpUpgradeContext and should be documented in the migration guide. Although the HttpUpgradeContext is not even mentioned in the docs.

No, but this context is used there without identity - https://quarkus.io/version/main/guides/websockets-next-reference#inspect-andor-reject-http-upgrade. I think it is breaking change that has very low impact since the HTTP checks are reactive so it is easy change

@mkouba
Copy link
Contributor Author

mkouba commented Oct 15, 2024

This is a breaking change in the HttpUpgradeContext and should be documented in the migration guide. Although the HttpUpgradeContext is not even mentioned in the docs.

No, but this context is used there without identity - https://quarkus.io/version/main/guides/websockets-next-reference#inspect-andor-reject-http-upgrade. I think it is breaking change that has very low impact since the HTTP checks are reactive so it is easy change

Agreed. Also the WS Next API is still "experimental" :D

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 is right change as in the past we proactively authenticated even though proactive auth was disabled. WS Next behavior will be similar to what other stacks do.

Copy link
Member

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @mkouba, and @michalvavrik for answering the question I was about to ask Martin :-), what impact this change would have...

@sberyozkin sberyozkin merged commit 2c9a5ac into quarkusio:main Oct 16, 2024
20 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.17 - main milestone Oct 16, 2024
@gsmet
Copy link
Member

gsmet commented Oct 16, 2024

@mkouba is it something you want for 3.16? If so, we need to add the backport label as it missed the CR1 train.

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