-
Notifications
You must be signed in to change notification settings - Fork 6k
Ease controllers unit tests in OAuth2 secured apps #6557
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
Comments
@ch4mpy Thanks for sharing this with us! We are definitely in need of OAuth 2.0 Client test support. Quick question regarding above...
We would welcome a PR targeted for the upcoming release of Spring Security 5.2.0 for initial test support for OAuth 2.0 Client. Would you be interested? Related #4833 |
@jgrandja, sure, I am interested. My first guess was you might be interested in keeping I'll try to have a look at what is related to token services in Spring-security 5.2.0 |
@ch4mpy Regarding...
Both |
Good news !I had time this week-end to get a closer look at spring-security 5 OAuth2 implementation and re-think the code I had written a year ago for spring-security-oauth. This led me to create a new lib: This is available in a forked spring-security repo, on the I also added a @Test
@WithMockJwtAuthentication
public void test() throws Exception {
api.perform(get("/"))
.andDo(print())
.andExpect(status().isOk())
.andExpect(content().string(is("Hello, " + WithMockJwtAuthentication.DEFAULT_CLAIMS_SUBJECT + "!")));
} ButI have still something pending: find a way to register a @jgrandja , Unfortunately I'm about to sail from Guadeloupe to Cuba with only a short stop in Antigua. It means I could not have any internet connection until I reach Jamaica by the end of the month. Can I get a quick feeling about this few commits ? |
Poor guy, @ch4mpy, having to sail to all those exotic places... ;) |
@ch4mpy Thanks for putting this together. I'd like to keep the PR as small and focused as possible so let's just focus on My initial thought is, should Also, |
As, as far as I understood, Spring 5 provides with only two OAuth2 authentication implementations. I did an annotation (and a security-context factory) for each, but sure, I can separate PRs.
The names might be too long and could be changed to
The reason why I named the annotations Maybe I misunderstand your remark but, as a unit-test writer, all I need is the ability to control the security-context authentication each As a side note, In this PR, I'm interest in building a Regarding claims:
With named properties in the annotation, I knew what the value should be turned into (so instantiated JWT claims actually are But should really a developer introduce a dependency on JWT headers from a controller ? Maybe could we just drop headers configuration from this PR ? Maybe could we also simplify claims configuration? For instance, |
@ch4mpy Regarding my comment...
Just to clarify so I don't miscommunicate here, I see that you have a lot of questions but it would be much easier to answer these within a PR. So my suggestion is to prepare/submit a PR for |
|
Added a commit with a solution to headers and claims values parsing from I couldn't delay my departure anymore. So one more wifi connection some night next week then nothing for three weeks or so => do not necessarily wait for my feedback... |
Sorry that I'm late to the party, but the above comment is why I believe "Please supply the security context with an authentication whose principal is of type And so "Please supply the security context with an authentication whose principal is of type We can add
Agreed, though I would go a step further: what an application typically needs is the principal. If the user specifically needs an instance of
If I'm understanding the context correctly, the answer here is "yes". The tester should be able to supply the contents of the would-be Jwt as part of the annotation. Annotations like
The length is a little off-putting, but I believe this is more about aligning to the semantics of |
Latest commits were prepared offline. Will integrate todays comments in an other offline session ;) |
Thanks, @ch4mpy. I've started looking through the PR, and it really feels very cumbersome to specify so much through an annotation. It makes me feel like it's not the right spot to be specifying claims and headers. This drives me to something like: @WithMockJwt(token="ey...", authorities={...}) instead. But, then, that would end up exercising a fair amount of Spring Security's jwt support to decode the token... and it starts feeling like an integration test. My next step, then, was to take that out, too, which simply leaves us with: @WithMockJwt(name="sub", authorities={...}) which seems rather weak. So, I chatted with @rwinch about this today, and he brought up an idea which I liked, which is to instead do a this.mvc.perform(get("/")
.with(jwt(instanceOfJwt).authorities(...)) and maybe this.mvc.perform(get("/")
.with(jwt().claim(SUB, "sub").authorities(...)) And, ostensibly, the reactive equivalent using One of the big benefits of this approach is users don't have to suffer specifying claim values under the type limitations that annotations impose. Another benefit is the test support footprint would be smaller, making it easier to maintain. I've marked the ticket for team discussion to see if @fhanik, @jgrandja, and @rwinch have thoughts one way or another. |
I second what @jzheaux has said above. I really think that adding |
What commit did you look at?
Only A few samples of current solution will illustrate better than many words : //all 5 tests pass in `spring-security-samples-boot-oauth2resourceserver` project
// Default name, empty authorities. Not of much use beyond "isAuthenticated()"
@Test
@WithMockJwt()
public void testDefaultJwt() throws Exception {
mockMvc.perform(get("/")).andDo(print()).andExpect(status().isOk()).andExpect(
content().string(is("Hello, " + WithMockJwt.DEFAULT_AUTH_NAME + "!")));
}
// Specify authorities only, keep default name
@Test
@WithMockJwt({ "ROLE_DEV", "CONTROLLER_UNIT_TESTER" })
public void testWithCustomAuthoritiesJwt() throws Exception {
mockMvc.perform(get("/")).andDo(print()).andExpect(status().isOk())
.andExpect(content().string(is("Hello, " + WithMockJwt.DEFAULT_AUTH_NAME + "!")));
}
// Specify authentication name (and token subject claim), empty authorities
@Test
@WithMockJwt(name = "ch4mpy")
public void testWithCustomNameJwt() throws Exception {
mockMvc.perform(get("/")).andDo(print()).andExpect(status().isOk())
.andExpect(content().string(is("Hello, ch4mpy!")));
}
@Test
@WithMockJwt(name = "ch4mpy", authorities = { "ROLE_DEV", "CONTROLLER_UNIT_TESTER" })
public void testWithCustomNameAndAuthoritiesJwt() throws Exception {
mockMvc.perform(get("/")).andDo(print()).andExpect(status().isOk())
.andExpect(content().string(is("Hello, ch4mpy!")));
}
// Assumes the controller accesses "just_a_string" and "important_instant" JWT claims
@Test
@WithMockJwt(
name = "ch4mpy",
authorities = { "ROLE_DEV", "CONTROLLER_UNIT_TESTER" },
claims = {
@Property(name = "just_a_string", value = "abracadabra"),
@Property(name = "important_instant", value = "2019-03-13T02:42:51Z", parser = "org.springframework.security.test.context.support.oauth2.properties.InstantPropertyParser") })
public void testWithAdvancedCustomJwt() throws Exception {
mockMvc.perform(get("/")).andDo(print()).andExpect(status().isOk())
.andExpect(content().string(is("Hello, ch4mpy!")));
} It would be similar for an app secured with Bearer access tokens: @Test
@WithMockOAuth2AccessToken(
name = "ch4mpy",
authorities = {"ROLE_DEV", "CONTROLLER_UNIT_TESTER"},
claims = {
@Property(name = OAuth2IntrospectionClaimNames.SCOPE, value = "truc", parser = "org.springframework.security.test.context.support.oauth2.properties.StringSetPropertyParser"),
@Property(name = OAuth2IntrospectionClaimNames.SCOPE, value = "chose", parser = "org.springframework.security.test.context.support.oauth2.properties.StringSetPropertyParser")})
public void testWithAdvancedCustomAccessToken() throws Exception { ... }
I used a pretty generic name: Things I did offline (before I could read latest comments):
P.S. I'm leaving Antigua / Barbuda tonight. It is very likely that I won't have a chance to read from you nor push anything for the next three weeks. |
The unsquashed equivalent of the most recent one. The current PR requires a host of conversion utilities and a new annotation (
It's not about the number, but what's going into them.
Annotations are still very limiting for this kind of thing either way. I immediately run into the same problem with Another benefit to a request post processor is that the user can easily extend the behavior: @Test
public void myTest() {
this.mvc.perform(get("/").with(userJwt("bob")));
}
private static RequestPostProcessor userJwt(String user) {
return jwt().claim(SUB, user).claim("user_id", user);
} which Java gives us for free. Java doesn't support this kind of thing with annotations. P.S.: I envy you! Enjoy your well-deserved trip, sir. :) |
I tried the following and it works (following test passes): @GetMapping("/test")
public String getTest(@AuthenticationPrincipal final Jwt principal) {
final Instant iat = (Instant) principal.getClaims().get("iat");
return "Claim is an Instant with value: " + iat.toString();
} Then you can edit https://github.com/ch4mpy/spring-security/blob/gh-6557/samples/boot/oauth2resourceserver/src/test/java/sample/OAuth2ResourceServerControllerTest.java and add: @Test
@WithMockJwt(
claims = @Property(
name = "iat",
value = "2019-01-01T13:45:45Z",
parser = "org.springframework.security.test.context.support.oauth2.properties.InstantPropertyParser"))
public void testInstantClaim() throws Exception {
mockMvc.perform(get("/test"))
.andExpect(content().string(is("Claim is an Instant with value: 2019-01-01T13:45:45Z")));
}
This required a few years to organize things, leave everything (job included) and prepare a boat, but, wow, if you could see my desk today... West Indies definitely are a dream place for sailing. |
@ch4mpy, I understand that your PR supports dates. My point is that doing so through an annotation is painful. I'd like the focus of this conversation to move from "can we" to "should we". I think that the annotation approach here is too cumbersome, so "can we"? Yes. "Should we"? Not at this point. Can you explain to me why you feel we should add this annotation support over doing a request post processor? Please see if reading my last few comments illustrates why I believe that a request post processor will have much less friction for the user and a much smaller test support code footprint. |
Could read your message a few more times offline without the pressure of WiFi plan count down and got your point about turning around annotations limitations late. Using parsers is the sole solution I found to custom claims which I believe is a must have. Again if a team puts custom claims in a token, it is very likely to use it somewhere in a controller or service component. Now, regarding the way I retrieve and instantiate parsers, there are different ways to go. I used
A simpler way to go I like is to just drop it and type "parser" as @claims = {
@Attribute(name="scope", value="message:read", parser=StringListParser.class),
@Attribute(name="ownedEntities", value="1,42,51", parser=CsvAttributeParser.class) } Thinking of it longer, I believe it is a better way to go (actually, I don't even see any good reason to keep the design as it is), provided you agree that custom claims are needed and reflecting a parser to de-serialize a value is considered an acceptable solution. |
Hi there ! Sorry for long silence, I was not allowed to stop at wish to find internet connection. I'm really exited to share my latest work. I spent way more time on this lib than I initially thought, but I feel the result is worth the effort. @jzheaux, did you have any opportunity to talk with @jgrandja who initially mentioned the need to support custom claims and headers (comment from March the 5th)?
@jzheaux, What do you think of my updated This long offline session gave me time to refine existing code and augment the lib. I worked on quite a few axes:
This leads to 10 branches and many potential trees, so please tell me:
As first guess and because, when I left, there where still ongoing discussions about their scope (should it support custom claims or not), I put annotations last:
Check-out this latest branch allows to play with any feature. Unit-tests and sample projects are perfect sand-boxes. Do not hesitate to ask for different branch organization. As I've already been rebasing a lot lately, I'm quite familiar with related STS & git commands... |
If anybody gets there some day, it's now faster to start from https://github.com/ch4mpy/spring-addons/tree/master/spring-security-test-oauth2-addons. It contains flow APIs for both JWT and Introspection. It also exposes annotations in case you need it to test secured P.S. I publish this lib on maven-central. |
I haven't read the whole comments yet but want to give my 2 cents to the naming of the annotation. In my opinion @WithMockJwtAuthentication or @WithMockJwt is not specific enough since JWT is only a format and there are different tokens in JWT format around in OAuth2. Namely access tokens, id tokens, refresh token. So I would suggest something like @WithMockAccessToken for the annotation. |
We avoided the issue in Spring Boot 2.1.x of no JWT support by just disabling security in the test (which is marked as deprecated): @WebMvcTest(controllers = MyController.class, secure = false) however, this was removed in Spring Boot 2.2.x, so we cannot upgrade. What is the roadmap/target release for this fix? |
@rnavarropiris JWT authentication testing support is integrated in spring security 5.2 with "flow" API (see samples) For annotation (or introspection) support you can see this lib |
@rnavarropiris, to add to @ch4mpy's comment, Spring Security's MockMvc support for JWTs is documented in the reference. |
Just in case someone needs it: Link to docs about MockMvc support for JWT has moved to here. |
We aren't going to be adding support for custom claims via annotation-based security at this time. Since this appears to be a core part of this proposal, I'm closing this ticket. If there is some gap identified that can't be addressed otherwise, we can reopen this ticket or create a new one. Note that |
Summary
I faced a few difficulties unit testing my controllers in a RESTful app secured with OAuth2 (JWT) and wrote a lib that quite improved my developer experience. Just wanted to share this work, maybe will you pick some ideas / code ?
What I did can be reduced to a few steps:
@WithMockOauth2Client
and@WithMockOauth2User
(later relies on first for client configuration and on@WithMockUser
for username and password configuration)ResourceServerTokenServices
to intercept specific Authorization headers and populate OAuth2 security context according to authentication described with preceding annotationsMockMvc
to add a specific Authorization header to the request when any of the two annotations described at step 1. was usedMockMvc
to:4.1. add
Content-type
header for eachPOST
,PUT
andPATCH
request4.2. add
Accept
header for eachGET
,POST
andOPTION
request4.3. provide with fine grained
MockHttpServletRequestBuilder
factories (pre-configured for a get request, a post request with a body, etc.)4.4. provide with shortcuts to create, configure, build and perform mocked MVC requests in one call
4.5. auto serialize requests payloads according to Content-type using registered message converters (see SerializationHelper)
Actual Behavior
Considering communities threads (stackoverflow being a sample), unit testing controllers in an app secured with OAuth2 is commonly considered as a painful task.
Expected Behavior
MockMvc
Sample
Overall result in some controller unit tests:
In this sample:
api
is aMockMvc
wrapper instanceMockHttpServletRequestBuilder
is created, configured, build and performed in one callP.S.
This is my first request to Spring framework, please point me to the right instructions if I do it the wrong way
The text was updated successfully, but these errors were encountered: