Skip to content

SecurityContextRequestPostProcessorSupport small-improvements #6857

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
ch4mpy opened this issue May 11, 2019 · 4 comments
Closed

SecurityContextRequestPostProcessorSupport small-improvements #6857

ch4mpy opened this issue May 11, 2019 · 4 comments
Assignees
Labels
in: test An issue in spring-security-test status: invalid An issue that we don't feel is valid

Comments

@ch4mpy
Copy link
Contributor

ch4mpy commented May 11, 2019

  1. SecurityContextRequestPostProcessorSupport does not define abstract method. There is no need for it to be abstract
  2. SecurityContextRequestPostProcessorSupport is stateless. There is no need for it to be a parent class, it can be a collaborator or an interface instead
  3. both SecurityContextRequestPostProcessorSupport and TestSecurityContextRepository could be of great use to framework users wishing to extend testing features.

I'm asking to change SecurityContextRequestPostProcessorSupport an TestSecurityContextRepository visibility to public.

I also propose to modify existing request post-processors so that it use SecurityContextRequestPostProcessorSupport as collaborator instead of extending it.

@rwinch
Copy link
Member

rwinch commented May 13, 2019

Thank you for reaching out @ch4mpy Can you describe what you are trying to achieve that is not simple at this time?

@rwinch rwinch added in: test An issue in spring-security-test status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels May 13, 2019
@ch4mpy
Copy link
Contributor Author

ch4mpy commented May 15, 2019

@rwinch I can't contribute test MockMVC request post-processor extending something else (breaks existing patterns as all other security related RequestPostProcessors extend SecurityContextRequestPostProcessorSupport).

Use case at the very bottom of https://github.com/ch4mpy/spring-security/blob/gh-6634--jwt-reactive-flow/test/src/main/java/org/springframework/security/test/web/servlet/request/SecurityMockMvcRequestPostProcessors.java, JwtRequestPostProcessor definition

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels May 15, 2019
@ch4mpy
Copy link
Contributor Author

ch4mpy commented May 17, 2019

@rwinch, I won't be able any longer to execute changes you would request. When opening this ticket, I thought a week would have been enough to solve it. I now have to go at sea, with almost no Internet connection for the coming two month, sorry. I hope I managed to give modification rights to Spring team.

Please note there are two commits on this branch:

  • first removes all dependencies from request post-processors to SecurityContextRequestPostProcessorSupport as parent and add it as collaborator on the only few requiring it
  • second holds visibility modifications and moves of newly public classes to their own files

@rwinch rwinch added status: invalid An issue that we don't feel is valid and removed status: feedback-provided Feedback has been provided labels May 20, 2019
@rwinch rwinch self-assigned this May 20, 2019
@rwinch
Copy link
Member

rwinch commented May 20, 2019

I'm closing this as invalid because user's can easily delegate to the existing public APIs to achieve the same things. For example:

@Override
public MockHttpServletRequest postProcessRequest(MockHttpServletRequest request) {
	Authentication authentication = ...;
	return SecurityMockMvcRequestPostProcessors.authentication(token).postProcessRequest(request);
}

@rwinch rwinch closed this as completed May 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: test An issue in spring-security-test status: invalid An issue that we don't feel is valid
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants