-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Security fixes for RESTEasy Reactive #14563
Conversation
stuartwdouglas
commented
Jan 25, 2021
- Add ability to deny access to unannotated endpoints
- Make sure setSecurityContext changes the current identity
- Port tests from RESTEasy extension
Injection of the SecurityContext will result in using the same SecurityContext for all requests.
- Add ability to deny access to unannotated endpoints - Make sure setSecurityContext changes the current identity - Port tests from RESTEasy extension
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
I just added some very minor comments
import io.quarkus.runtime.annotations.ConfigRoot; | ||
|
||
/** | ||
* @author Michal Szynkiewicz, [email protected] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe drop these author tags which are incorrect anyway?
private CurrentIdentityAssociation getCurrentIdentityAssociation() { | ||
CurrentIdentityAssociation identityAssociation = this.currentIdentityAssociation; | ||
if (identityAssociation == null) { | ||
return this.currentIdentityAssociation = CDI.current().select(CurrentIdentityAssociation.class).get(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably best to be consistent and use Arc.container()
instead of CDI.current()
Also I believe this should be backported, right? |
I think we really need this in for 1.11.1.Final. Looks like the comments are mostly about details? I can fix them myself or we can merge as is. @geoand WDYT? |
Sure yeah, we can definitely merge as is |
OK, let's merge it then. This can't stay as is. |