-
Notifications
You must be signed in to change notification settings - Fork 6k
-gh 8784 Document improvement for WebSecurityConfigure #8825
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
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.
Thanks, @romilptl, for the PR! I've left some feedback inline.
@@ -330,6 +330,12 @@ public void init(final WebSecurity web) throws Exception { | |||
/** | |||
* Override this method to configure {@link WebSecurity}. For example, if you wish to | |||
* ignore certain requests. | |||
* | |||
* Endpoint used in this method ignores the spring security filters, security features (secure headers, csrf protection etc) are also ignored |
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.
To make the documentation easier to digest, will you please break it up into multiple sentences? Also, will you please break the lines a little earlier to match the rest of the JavaDoc here?
For example, you might do something like:
Endpoints specified here will not be protected by Spring Security. This means that ...
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.
You might also add something like:
Instead, if you want to protect public endpoints against common vulnerabilities, then see
{@link #configure(HttpSecurity)} and the {@link HttpSecurity#authorizeRequests}
configuration method.
@@ -330,6 +330,12 @@ public void init(final WebSecurity web) throws Exception { | |||
/** | |||
* Override this method to configure {@link WebSecurity}. For example, if you wish to | |||
* ignore certain requests. | |||
* | |||
* Endpoint used in this method ignores the spring security filters, security features (secure headers, csrf protection etc) are also ignored | |||
* and no security context will be set and can not protect endpoints for Cross Site Scripting, XSS attacks, Content-Sniffing etc |
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.
I think that Cross-Site Scripting and XSS attacks are the same. Is there a reason to include both on the list?
* Endpoint used in this method ignores the spring security filters, security features (secure headers, csrf protection etc) are also ignored | ||
* and no security context will be set and can not protect endpoints for Cross Site Scripting, XSS attacks, Content-Sniffing etc | ||
* | ||
* @see <a href="https://docs.spring.io/spring-security/site/docs/current/reference/html5/#csrf">Cross Site Request Forgery (CSRF)</a> |
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.
It's tricky to include links to the reference manual in the JavaDoc. If you point to a specific revision, then the links have to be updated with each release. If you point to current
, then the link has to be updated any time the documentation links change.
Instead, I'd recommend linking to CsrfConfigurer
and HeadersConfigurer
, respectively.
@@ -343,6 +349,12 @@ public void configure(WebSecurity web) throws Exception { | |||
* http.authorizeRequests().anyRequest().authenticated().and().formLogin().and().httpBasic(); | |||
* </pre> | |||
* | |||
* Endpoint used in this method with permitAll() ignores the authentication |
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.
I think this statement isn't entirely true since the application can disable headers, CSRF, etc.
Maybe something like:
Public endpoints that require defense against common vulnerabilities can be specified here.
See {@link HttpSecurity#authorizeRequests} and the `permitAll()` authorization rule
for more details.
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.
Thanks for the feedback and details. I have done the changes accordingly.
Both methods configure(WebSecurity web) and configure(HttpSecurity http) provides a mechanism to manage the authentication for the API endpoints.
Added details about how the authentication mechanism works with other security features for public APIs
Issue: #8784