Skip to content

Conversation

marcusdacoregio
Copy link
Contributor

Create SecurityMockMvcResultHandlers to define security related MockMvc ResultHandlers
Create a method to allow copying the SecurityContext from the TestSecurityContextHolder to SecurityContextHolder

Closes gh-9565

@marcusdacoregio marcusdacoregio added status: duplicate A duplicate of another issue in: test An issue in spring-security-test type: enhancement A general enhancement labels May 10, 2021
@marcusdacoregio marcusdacoregio added this to the 5.6.x milestone May 10, 2021
@marcusdacoregio marcusdacoregio self-assigned this May 10, 2021
@marcusdacoregio marcusdacoregio force-pushed the gh-9565 branch 4 times, most recently from 69ce852 to 585aa5e Compare May 11, 2021 12:17
@marcusdacoregio marcusdacoregio requested a review from jzheaux May 31, 2021 16:08
@marcusdacoregio
Copy link
Contributor Author

A new section in the documentation named SecurityMockMvcResultHandlers should be added to show this new feature.

@marcusdacoregio marcusdacoregio force-pushed the gh-9565 branch 2 times, most recently from 4819ec4 to cc3d521 Compare June 30, 2021 18:41
@marcusdacoregio marcusdacoregio modified the milestones: 5.6.x, 5.6.0-M2 Jul 19, 2021
@rwinch rwinch modified the milestones: 5.6.0-M2, 5.6.0-M3 Aug 16, 2021
@marcusdacoregio marcusdacoregio requested a review from rwinch August 26, 2021 11:41
Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, @marcusdacoregio! Please see my feedback inline.

@marcusdacoregio
Copy link
Contributor Author

Thank you very much for your review @jzheaux.

I've updated the PR based on your suggestions.

Copy link
Member

@rwinch rwinch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I've left feedback inline.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like a name that is a bit more explicit as to what is happening. Perhaps propagateTestSecurityContext? I think a better name could be found that my suggestion, but the idea is it should convey what is being done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"propagate", "retain" and "keep" all seem to imply that it will be retained across multiple MockMvc invocations in the same test. Would that be the case? If this isn't a concern, then my favorite among these options is "retain". If it is a concern, then I still like "export" best.

Looking more deeply into the support, I see that there is a request post-processor static method called testSecurityContext(). In that case, "xyzTestSecurityContext" does seem more consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should indeed mention TestSecurityContext in the name. It would be explicit if we do exportTestSecurityContext, since it gives the idea of the TestSecurityContext being exported somewhere, and this also gives the opportunity to extend it to exportTestSecurityContext(Consumer<SecurityContext> exportTo).

If we use names like save, propagate, preserve, all of them give the idea that the TestSecurityContextHolder will not be cleared out.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"propagate", "retain" and "keep" all seem to imply that it will be retained across multiple MockMvc invocations in the same test. Would that be the case?

I am not a fan of save, retain, preserve, or keep because I interpret these to mean that the TestSecurityContextHolder will not be changed which is true regardless of the ResultHandler being used. The behavior is copy/export/propagate the TestSecurityContextHolder into the SecurityContext.

Create SecurityMockMvcResultHandlers to define security related MockMvc ResultHandlers
Create a method to allow copying the SecurityContext from the TestSecurityContextHolder to SecurityContextHolder

Closes spring-projectsgh-9565
@marcusdacoregio
Copy link
Contributor Author

marcusdacoregio commented Sep 17, 2021

Thanks @rwinch. I've updated the PR based on your suggestions.

The method's name has been changed to exportTestSecurityContext.

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: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Propagate TestSecurityContextHolder to SecurityContextHolder after MockMvc calls
3 participants