-
Notifications
You must be signed in to change notification settings - Fork 161
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
CCDM: check userPrincipal for non-anonymous methods #6782
Conversation
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.
Reviewed 2 of 2 files at r2.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @qtdzz)
flow-server/src/main/java/com/vaadin/flow/server/connect/VaadinConnectController.java, line 251 at r2 (raw file):
@PathVariable("method") String methodName, @RequestBody(required = false) ObjectNode body, HttpServletRequest request) {
I feel that we should use VaadinRequest instead, so as developer can set Principal and Roles somehow from the session, or trust in default HttpServletRequest
.
HttpServletRequest
is an interface and the implementation depends on the servlet container flavour, I don't think it's easy for the user modify the request to set for intance the principal based on session
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.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @manolo)
flow-server/src/main/java/com/vaadin/flow/server/connect/VaadinConnectController.java, line 251 at r2 (raw file):
Previously, manolo (Manuel Carrasco Moñino) wrote…
I feel that we should use VaadinRequest instead, so as developer can set Principal and Roles somehow from the session, or trust in default
HttpServletRequest
.
HttpServletRequest
is an interface and the implementation depends on the servlet container flavour, I don't think it's easy for the user modify the request to set for intance the principal based on session
The connect request is not handled by VaadinServlet but the Spring controller. I don't think that there is a way to get VaadinRequest from here.
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.
Reviewed 5 of 7 files at r1.
Reviewable status: all discussions resolved, 0 of 1 LGTMs obtained
flow-server/src/main/java/com/vaadin/flow/server/connect/VaadinConnectController.java, line 251 at r2 (raw file):
Previously, qtdzz (Tien Nguyen) wrote…
The connect request is not handled by VaadinServlet but the Spring controller. I don't think that there is a way to get VaadinRequest from here.
Good point.
Then when user needs custom authentication, probably they need to provide appropriate HttpServletRequest
in a filter.
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.
Reviewable status: complete! all discussions resolved, 1 of 1 LGTMs obtained
Connected to #6754
Use
accessChecker
to verify the method accessibility. If the method/class is annotated with@AnonymousAllowed
, the method is publicly available. Otherwise,HttpServletRequest.getUserPrincipal()
should return a not null Principal object to access the method.This change is