Skip to content

HttpStatusServerAccessDeniedHandler doesn't work as intended #5078

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

Closed
edeandrea opened this issue Mar 7, 2018 · 9 comments
Closed

HttpStatusServerAccessDeniedHandler doesn't work as intended #5078

edeandrea opened this issue Mar 7, 2018 · 9 comments
Assignees
Labels
in: web An issue in web modules (web, webmvc) type: bug A general bug
Milestone

Comments

@edeandrea
Copy link
Contributor

Summary

I'm trying to convert my current servlet-based application over to reactive and it seems some of the Spring Security support for the reactive stack isn't yet complete.

  1. The API documentation (class-method level documentation) seems to be missing in lots of places (ServerHttpSecurity and all its nested classes/methods, SecurityWebFilterChain, HttpStatusServerAccessDeniedHandler, plus there are probably others that I haven't yet come across)
  2. HttpStatusServerAccessDeniedHandler in particular the documentation is not complete. It also reads in an HttpStatus to set, but then the handle method never uses it. Line 41 shows response.setStatusCode(HttpStatus.FORBIDDEN);

Expected Behavior

I would expect that whatever HttpStatus I construct the HttpStatusServerAccessDeniedHandler with would be the status code on the outgoing response.

Version

5.0.3.RELEASE

@rwinch rwinch added this to the 5.0.4 milestone Mar 7, 2018
@rwinch rwinch self-assigned this Mar 7, 2018
@rwinch rwinch added the type: bug A general bug label Mar 7, 2018
@rwinch rwinch added in: web An issue in web modules (web, webmvc) Reactive labels Mar 7, 2018
@rwinch rwinch modified the milestones: 5.0.4, 5.1.0.M1 Mar 7, 2018
@rwinch rwinch closed this as completed in 9f23212 Mar 7, 2018
@rwinch
Copy link
Member

rwinch commented Mar 7, 2018

Thanks for the report. I pushed a fix for the HttpStatus to master and 5.0.x. We have an on going issue to clean up Javadoc in the reactive bits. If you have specific classes that you need prioritized please create a separate issue for that

@edeandrea
Copy link
Contributor Author

edeandrea commented Mar 7, 2018

Thanks @rwinch

One thing I've noticed is it seems on the reactive side lots of the basic building-blocks that are there on the servlet side are missing.

Things like

  • Alternate implementations of ServerLogoutSuccessHandler (like HttpStatusReturningLogoutSuccessHandler)
  • Alternate implementations of ServerAuthenticationEntryPoint (like HttpStatusEntryPoint or Http403ForbiddenEntryPoint)
  • Cookie-based implementation of ServerCsrfTokenRepository (like CookieCsrfTokenRepository with the ability to set the domain on the cookie)

Is this by design? Are these kinds of things not-needed in a reactive environment?

@rwinch
Copy link
Member

rwinch commented Mar 7, 2018

@edeandrea Thanks for the report. We know that there is still a lot to do on the reactive side. It was impossible to replicate all of the code within the servlet world (which was nearly 15 years of work) within a single release. Please create tickets for specific things you would like to see and we will prioritize it. Please avoid a long list if items, but instead create individual tickets.

@edeandrea
Copy link
Contributor Author

Great thanks @rwinch . I just wasn't sure if this was by design or because of what you just mentioned - that it just hasn't yet caught up.

I'll submit additional tickets.

@edeandrea
Copy link
Contributor Author

I opened #5081, #5082, & #5083.

@rwinch
Copy link
Member

rwinch commented Mar 7, 2018

Thanks @edeandrea! Any chance you would be willing to submit PRs for any of those tickets?

@edeandrea
Copy link
Contributor Author

I would totally be open to it as I'm going to have to build them myself anyways (I've already built the cookie csrf token repository, but will have to check with my company's legal team to see if thats ok (I work for a big & old company.... :( )

@rwinch
Copy link
Member

rwinch commented Mar 7, 2018

@edeandrea Thanks for looking into it! Let me know either way. If you can do it, that would be a great help. If you cannot do it I will be sure to prioritize it myself

@edeandrea
Copy link
Contributor Author

I got approval from my company's legal team to contribute - I added PR 5245 for #5083

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web An issue in web modules (web, webmvc) type: bug A general bug
Projects
None yet
Development

No branches or pull requests

2 participants