mockJwt() flow API for reactive server

[ch4mpy]

request: #6634
initiated by: #6557

[jzheaux]

Thanks, @ch4mpy! (And welcome back from your trip!) I've left some feedback inline.

[ch4mpy]

Thanks, @ch4mpy! (And welcome back from your trip!) I've left some feedback inline.

I'm about to leave for a new leg: Cuba -> Jamaïca. This should be way shorter but as winds and currents are not favorable, might not arrive before next week-end. Hope you're not in too much of a hurry for my corrections

[jzheaux]

Thanks for your responses, @ch4mpy. I've provided some more feedback when you have a moment.

[jzheaux]

I hear your concern - you have other PRs that you anticipate will share logic down the road and you don't want to shift the code around needlessly.

It's not clear that everything in here will be necessarily shared. We don't want to abstract too early. Further, we try and favor composition over inheritance, and so we're especially cautious about introducing an abstract class in a PR with only one extending class.

Second, it's not clear when things will get merged. You've already seen a few weeks go by - these things take time. We don't want to create unnecessary pressure to merge additional PRs in order to prove out an abstraction before it goes GA.

jwt() is not enough

We have to start somewhere. Perhaps the best route is to get JWT out the door as well as make it easier for the community to build extensions, as you alluded to in another comment. I believe that we can get more than JWT done, but anticipating it in this PR is probably premature.

And you might be right on your predicated abstraction. I'll often create several branches on my local machine with additional abstraction work that I don't want to lose, and then apply it when the time comes. Sometimes, I'm proven right on my predicted abstraction. Other times, I'm wrong.

[jzheaux]

Actually, HashSet constructor expects a Set, while Arrays.asList provides, with ... a List

That's not actually correct. HashSet expects a Collection.

Thank you for cleaning this up.

[jzheaux]

The framework parses scopes in order to obtain GrantedAuthorities. Insofar as the purpose of this support is to bypass authentication piping, it makes sense to simply allow the user to supply the authorities directly.

[jzheaux]

I really appreciate your additional contributions, and I believe we will get to them in time.

[jzheaux]

This seems to fit better in Jwt itself, alongside the other validation Jwt does in its constructor. Do you see this validation as a better fit in the testing support?

[ch4mpy]

I dropped it from this PR, so it is possible to build incoherent tokens. In such case, claim issue and expiry dates win.

I'm a bit confused with AbstractOAuth2Token. Why are issuedAt and expiresAt members? It is a duplicate for corresponding claims at so many places.

I feel like a final Map<String, Object> attributes would be a better candidate for abstraction (don't all token have some, it sometimes being called claims?).

I wonder also if getIssuedAt() and getExpiresAt() wouldn't better be abstract in AbstractOAuth2Token and accessed from claims from each token type, using corresponding claim names.

What do you think? Should a ticket be created for that?

[jzheaux]

Thanks for adding a ticket for this - I think it's a valuable discussion.

[ch4mpy]

@jzheaux, @jgrandja, I did: #6807
but it was closed "won't fix" :-/ Feel free to re-open if relevent.

Adding the comment I put below to that ticket to provide more details on my intentions.

[jzheaux]

Thanks again, @ch4mpy. I've left some feedback inline.

Note that a number of them are simple housekeeping items related to bring the coding style in line with the rest of the codebase. Generally speaking:

• If it isn't needed for this PR, leave it out, and
• Ideally, follow existing patterns of related code

[jzheaux]

This is a feature that I believe ties the framework too tightly to how authorities are derived. If a tester wants to use an authorities converter, they can simply invoke that on their own and then provide that fully-constructed Jwt.

[ch4mpy]

Converter is used the other way around. It is required to create authorities out of:

• fully-constructed Jwt
• claims

authoritiesConverter is defaulted to JwtGrantedAuthoritiesConverter which means most of the time, tester won't have to configure it.

side note: if configuration was registering a Converter<Jwt, Collection<GrantedAuthority>> bean (JwtGrantedAuthoritiesConverter being the implementation), we could @Autowire it both here and in JwtAuthenticationConverter. If this bean was a Converter<Map<String, Object>, Collection<GrantedAuthority>>, then, maybe, could it be used for any OAuth2 token type.

[jzheaux]

Please take a look at UserRequestPostProcessor#roles for hints on how the roles methods ought to work.

[ch4mpy]

Removed the existing roles cleanup, it now just adds to already registered ones as UserRequestPostProcessor#roles does.

Side note, I prefer the previous behavior: in case you have a method defining a custom authentication and want just a subset of the roles set for a certain test, then you are screwed.

[jzheaux]

I don't think I quite followed. Why couldn't you do:

MyCustomAuthentication authentication = new MyCustomAuthentication();
List<String> roles = authentication.getAuthorities().stream()
    .filter(this::predicate).map(Objects::toString).collect(toList());
JwtAuthenticationBuilder jwtAuthenticationBuilder = new JwtAuthenticationBuilder();
jwtAuthenticationBuilder.roles(roles);

How did I misunderstand your use case?

[ch4mpy]

Well, that demonstrates MyCustomAuthentication is hardly customizable...

Following the "last call wins" principle, I'd expect .roles("a", "b").roles("c") to configure authentication with ROLE_c only.

I add following test case to assert roles as you expect it to:

public class SampleTests {
	@Mock
	MockHttpServletRequest request;
	
	private JwtRequestPostProcessor defaultTestAuthenticationPostProcessor() {
		return jwt().name("test_user").roles("a", "b");//more like claims here too
	}
	
	@Test
	public void jwtRolesAreAddedAndNotReplaced() {
		final JwtRequestPostProcessor rpp = defaultTestAuthenticationPostProcessor().roles("c");
		final JwtAuthenticationToken actual = (JwtAuthenticationToken) authentication(rpp.postProcessRequest(request));
		
		assertThat(actual.getAuthorities()).containsExactlyInAnyOrder(
				new SimpleGrantedAuthority("ROLE_a"),
				new SimpleGrantedAuthority("ROLE_b"),
				new SimpleGrantedAuthority("ROLE_c"));
	}

	static Authentication authentication(final MockHttpServletRequest req) {
		final SecurityContext securityContext = (SecurityContext) req.getAttribute(TestSecurityContextRepository.ATTR_NAME);
		return securityContext == null ? null : securityContext.getAuthentication();
	}
}

The reason for me exposing both role(String role) and roles(String... roles) was proposing both behaviors:

• first adds to authorities
• second replaces role-authorities

I had implemented this behavior for all collections (roles, authorities, claims and scopes when it was still there). It should now all behave as asserted in this test (and that's a noticeable lost)

[jzheaux]

I'm not too keen on actually supporting this use case as this isn't part of constructing a JwtAuthenticationToken but instead a convenience that the authentication flow is providing. Generally, applications are going to work directly with GrantedAuthority instances, not with the scope claim, and so I don't see the reason to optimize for the use case where testers are hitting controllers that are checking the scope claim of the originating Jwt

[ch4mpy]

The test name changed to convertedClaimsAreMergedWithAuthorities as what I test here is that converter is actually triggered and all authorities are merged.

applications are going to work directly with GrantedAuthority instances, not with the scope claim

I think it is a dangerous assumption as applications might work with claims directly. Any claim. This can be achieved using SPeL or casting Authentication. I'm talking about use cases similar to what is done in OAuth2ResourceServerController from spring-security-samples-boot-oauth2resourceserver-opaque (I know JWT are not involved there, but I'm sure you can transfer the use-case)

[jzheaux]

@jgrandja @rwinch -

The question in my mind is: What is the user's expectation when doing this:

JwtAuthenticationToken token  = new JwtAuthenticationBuilder()
    .claim("scp", "read write").build();
token.getAuthorities().forEach(System.out::println)

What would the user expect to be displayed? Would it be zero, one, or two authorities?

Likewise, what would the user expect here:

JwtAuthenticationToken token  = new JwtAuthenticationBuilder()
    .authorities(new SimpleGrantedAuthority("SCOPE_read")).build();
System.out.println(token.getClaims().get("scp"));

Note that there may be some value in changing out roles(String...) in this PR for scopes(String...) and that there is precedent there for adding those Strings as GrantedAuthorities, though I'd like your feedback also on the value of somehow adding those as a claim.

I have my own opinion on each of these (hopefully my earlier comments don't color your opinions too much), but I'd like to hear your feedback before proceeding.

[ch4mpy]

Saving my opinion on a notebook, not to "color" others one, but would be happy to provide with my answers too

[jzheaux]

You make a good point about the opaque use case, and that is something we are in the process of cleaning up in #6498. Regardless, the canonical place to read authorities is from Authentication, not from something OAuth 2.0-specific.

I've had a quick chat with Rob offline about this to get his perspective (which I'm mentioning so it's clear why I'm responding without any posted comments from him or Joe).

As you have already alluded to, the transformation of the scope claim to a collection of GrantedAuthorities may change quite a bit from app to app. The simplest way for this API to support that is to keep them separate. The application can register a claim to the Jwt and an authority to the JwtAuthenticationToken to get the behavior they are wanting.

Of course, if we need to provide support to sync these up down the road, then we can add it.

[ch4mpy]

the canonical place to read authorities is from Authentication

Agree

The simplest way for this API to support that is to keep them separate

Disagree. The simplest way is to use the converter that is responsible for that in the app.

The application can register a claim to the Jwt and an authority to the JwtAuthenticationToken

Do you really want to write the doc stating "You can add the claims you like. You also can build a test JwtAuthenticationToken by building a fully configured Jwt and providing it to the test JwtAuthenticationToken. Oh, by the way, in both cases you also need to hand configure the authorities afterward because we thought, in case you're not using default one, it was too complicated for you to provide the authorities converter to the test framework ?

[ch4mpy]

I think I can now put the answers to your questions

.claim("scp", "read write") if default converter is not replaced, definitely ["SCOPE_read", "SCOPE_write"]. Rule being all claims should be processed by authorities converter.

.authorities(new SimpleGrantedAuthority("SCOPE_read")) should have no impact on claims. As converters are uni-directional, there is no reliable way to figure out claims from authorities using tested app converters. If a user wants claims to be set then he sets it (with an exception for subject claim)

scope claim to a collection of GrantedAuthorities may change quite a bit from app to app

I would rather say that for Jwt, scope is a detail, if not a poison. In our discutions, it's something we should carefully avoid (you were right to ask me remove it from builder API).

As far as I (now) understand the spec and spring implementation, there is no requirement for an app to use neither "scope" nor "scp" claims to provide resource-server with authorities. It perfectly makes sens to use stuff like "authorities" or "roles" or whatever, choose a fancy serialization or decide they just don't want the "SCOPE_" prefix.

As a consequence, I think we really have to keep the ability to configure the JwtAuthenticationTokenBuilder with the app authorities converter

• defaulting the converter to JwtGrantedAuthoritiesConverter, most users won't even notice it's there and the rare ones replacing it in their app will have no difficulty to provide it to the test framework
• with very little work on the configuration lib, you can define this authorities converter as a been which could then be just @Autowired

[jgrandja]

@jzheaux In response to your comment:

JwtAuthenticationBuilder().authorities()

I would expect this to add GrantedAuthority(s) to JwtAuthentication

JwtAuthenticationBuilder().claim()

Here I would expect claim(s) to be added to JwtAuthentication.Jwt.claims. I do not see GrantedAuthority(s) being added in addition.

I know the default behaviour of JwtAuthenticationConverter doesn't align with my statement here but IMO these are 2 different use cases.

JwtAuthenticationBuilder simply builds a JwtAuthentication, pretty straight forward. However, JwtAuthenticationProvider which composes JwtAuthenticationConverter provides more complex behaviour as it's responsible for authenticating a Jwt into a JwtAuthentication.

[jzheaux]

I think the documentation would read something like:

You can specify `GrantedAuthority`s the same way as other mock 
`Authentication` principals by calling the `authorities` method: {example} 

As a convenience, you could instead call `scopes`, which works the 
same as `roles`, just with the `SCOPE_` prefix instead of `ROLE_`: {example}

Since GrantedAuthority is the first-class citizen here, I doubt the docs would take the time to list the myriad ways the code chooses not to represent authorities.

Developers are smart - they can create their set of authorities in myriad ways and those that want to use a converter still can.

A very simple way that already exists today is authentication(JwtAuthenticationToken). For example, they can build a Jwt, wire their converter into their test and construct the token themselves, with only a view lines of code:

Jwt jwt = jwtConstruction();
Collection<GrantedAuthority> authorities = autowiredConverter.convert(jwt);
JwtAuthenticationToken token = new JwtAuthenticationToken(jwt, authorities);
this.mvc.perform(get().with(authentication(token)));

It's good to start with a minimal API and see what patterns emerge from the community at large. Really, there's nothing that says that something like authoritiesConverter can't be added later on.

[jzheaux]

We should try and favor composition over inheritance. One way to achieve this would be to, in the SecurityMockServerConfigurers and SecurityMockMvcRequestPostProcessors classes, doing:

public JwtMutator mockJwt(Consumer<JwtAuthenticationBuilder> jwtAuthenticationBuilderConsumer) {
    JwtAuthenticationBuilder jwtAuthenticationBuilder = new JwtAuthenticationBuilder();
    // ... any defaults necessary
    jwtAuthenticationBuilderConsumer.accept(jwtAuthenticationBuilder);
    return new JwtMutator(jwtAuthenticationBuilder);
}

[ch4mpy]

Could you please detail a bit more the JwtMutator implementation you're thinking of?

We should try and favor composition over inheritance

I agree with this. But "favor" isn't "avoid", right?

To me JwtMutator and JwtRequestPostProcessor are builders in the sens the "flow" API they implement is builder API: exposed methods incrementally mutate properties later used to build something.

Additionally, I don't quite know how to turn this "are builders" into "have builders" without proxying each and every builder method in both JwtMutator and JwtRequestPostProcessor. So if you could kindly give me a hint...

[jzheaux]

You are spot on that doing a delegate here would be a pain due to all the replicated methods.

If you expose the builder in mockJwt(), though, then you won't need to do that:

this.client.mutateWith(
    mockJwt(jwtAuthenticationBuilder -> {
        jwtAuthenticationBuilder.name("user"); // and whatever else
    }))
    // ...

JwtMutator (and JwtRequestPostProcessor by association) would then look like:

public class JwtMutator implements WebTestClientConfigurer, MockServerConfigurer {
    private final JwtAuthenticationBuilder jwtAuthenticationBuilder;

    // constructor, overrides

    private <T extends WebTestClientConfigurer & MockServerConfigurer> T configurer(...) {
        return mockAuthentication(this.jwtAuthenticationBuilder.build());
    }
}

[jzheaux]

And actually, I think a potential improvement on that would be to separate out the notion of creating a Jwt from creating an authentication token:

this.client.mutateWith(
    mockJwt(jwtBuilder -> jwtBuilder.claim(ISS, "https://idp.example.com"))
            .authorities(...))
    // ...

And then:

public class JwtMutator implements WebTestClientConfigurer, MockServerConfigurer {
    private final JwtBuilder jwtBuilder;
    private Collection<GrantedAuthority> authorities = AuthorityUtils.
            .createAuthorityList("SCOPE_USER");

    // constructor, overrides, authorities support

    private <T extends WebTestClientConfigurer & MockServerConfigurer> T configurer(...) {
        return mockAuthentication(new JwtAuthenticationToken(this.jwtBuilder.build(), this.authorities));
    }
}

And finally:

public static JwtMutator mockJwt(Consumer<JwtBuilder> jwtBuilderConsumer) {
    JwtBuilder jwtBuilder = new JwtBuilder();
    jwtBuilder.header("alg", "none");
    // ... other defaults
    jwtBuilderConsumer.apply(jwtBuilder);
    return new JwtMutator(jwtBuilder);
}

This has the benefit of focusing on the biggest complexity, creating the Jwt, and JwtBuilder seems really beneficial to promote up to oauth2-jose, for example as an inner class to Jwt. It would also make JwtRequestPostProcessor and JwtMutator quite reminiscent of UserRequestPostProcessor and UserExchangeMutator, which is often a good thing. It does have a very slight consequence of duplicating the authorities support in JwtRequestPostProcessor and JwtMutator.

[ch4mpy]

The more I'm thinking of it, the more I believe that what should be removed is authorities(...) from JwtAuthenticationTokenBuilder public API:

• authorities are not enough for JWT security: expressions can use claims that won't be converted to authorities (see sample below)
• claims always contain authorities (deserializing it correctly is just a matter of using the app authorities converter)
• storing authorities in addition to claims in JwtAuthenticationTokenBuilder leads to the same potential incoherence issues, we have inside Jwt for issue and expiration instants: it is bad idea to store two different representations of the same value (authorities).

Sample incoherent JwtAuthenticationTokenBuilder with claims and authorities
builder.claim("scp", "read").authorities(new SimpleGrantedAuthority("SCOPE_write") will be interpreted differently by

• @PreAuthorize("hasAuthority('SCOPE_read')) with JwtGrantedAuthoritiesConverter
• @PreAuthorize("#scopes.contains('read')") with @AuthenticationPrincipal(expression="claims['scp']") String scopes)

both expressions would behave the same with a real token

security expression not involving authorities

@RestController
public class OAuth2ResourceServerController {
    @PreAuthorize("#age >= 16")
    @GetMapping("/controlled")
    public String controlled(@AuthenticationPrincipal(expression="claims['age']") Integer age) {
        return "Hey mon, buy me drink.";
    }
}

@RunWith(SpringRunner.class)
@WebMvcTest(OAuth2ResourceServerController.class)
public class OAuth2ResourceServerControllerTest {
	@Autowired
	MockMvc mockMvc;

	@MockBean
	JwtDecoder jwtDecoder;
	
	@Test
	public void controlChecksIsOver16() throws Exception {
		mockMvc.perform(get("/controlled").with(jwt()))
				.andExpect(status().isForbidden());
		
		mockMvc.perform(get("/controlled").with(jwt().claim("age", 18)))
				.andExpect(content().string(is("Hey mon, buy me drink.")));
		
		mockMvc.perform(get("/controlled").with(jwt().claim("age", 15)))
				.andExpect(status().isForbidden());
	}
}

[jzheaux]

Thanks for all your research, @ch4mpy.

You've convinced me that scopes -> authorities is a reasonable default. As you said with your earlier comment, it's reasonable that a tester would just want to specify scopes and anticipate that the test support would interpret the scopes in what is already a pretty universal way.

I'd like to propose some modifications to your proposal via a few different use cases:

Authenticated with a JWT:

this.mvc.perform(get("/").with(jwt()));

This Jwt would have a default subject, algorithm, and scope claim. There would be a granted authority that coheres with the scope claim.

Authenticated with a JWT that has scopes:

jwt(jwt -> jwt.claim("scope", "read write"))

This Jwt would have a default subject and algorithm, and the indicated scope claim. There would be two granted authorities, "SCOPE_read" and "SCOPE_write".

The most opinionated is still to provide the authorities, so the following would override any claims in the Jwt:

jwt(jwt -> jwt.claim("scope", "not reflected as authorities"))
    .authorities("SCOPE_ADMIN")

This Jwt would have a default subject and algorithm, and the indicated scope claim. There would be one granted authority, "SCOPE_ADMIN".

But, a conversion strategy still makes sense, so the authorities could be overridden like so:

jwt(jwt -> jwt.claim("scope", scopes))
    .authorities(authoritiesConverter)

This Jwt would have the default subject and algorithm and the indicated scope claim. There would be whatever granted authorities came out of the conversion.

The point of the consumer is to separate the construction of the Jwt from the rest of the API. Separating the concern this way seems more natural than something like jwt().claim().authorities() where these concepts are melded together (something that harks back to your comment about wanting to remove the authorities method).

It also introduces JwtBuilder, which seems very reusable.

It lines up nicely with other places where this pattern is employed like how http headers and attributes are specified in WebClient:

WebClient.builder()
    .headers(headers -> headers.setBasicAuth(...))
    .attributes(attributes -> ...)

It also lines up with a feature under consideration via #5557 for the HttpSecurity DSL.

[ch4mpy]

I'll try to put all this together this weekend. Injecting the converter the way you ask is straight forward. Regarding the authorities override, I still need to figure out a clean way to do it, respecting the "last cal wins" principle. Will do my best for this to be pushed by Monday morning.

[ch4mpy]

What I did:

• create a Jwt.Builder to set token value, claims and headers
• create a JwtAuthenticationToken.Builder to configure the Jwt (.token(Consumer<Jwt.Builder> jwtBuilderConsumer)), authentication name and scopes (as specified by RFC-6749 : a "scope" claim, value being a spaced separated string)
• modify JwtAuthenticationTokenTestingBuilder to extend JwtAuthenticationToken.Builder, adding testing default values and authorities extensions
• .authorities(...) adds to converted authorities (does not replace it)

What I haven't done: modify JwtRequestPostProcessor extend SecurityContextRequestPostProcessorSupport. I created #6857 instead to modify exiting request post processors.

Reason for that is I believe authentication related MockMvc post-processors and WebTestClient configurers are easier to use if it is (extend) authentication builders (flow API) rather than collaborate with it through Consumer interface (JwtAuthenticationToken.Builder already collaborating with Jwt.Builder through this interface, that would be a lot of indirections).

Sample:

jwt(jwt -> jwt.claim("foo", "bar")).name("ch4mpy").scopes("TEST")

is easier to read than

jwt(jwtAuthenticationToken -> jwtAuthenticationToken.token(jwt-> jwt.claim("foo", "bar")).name("ch4mpy").scopes("TEST"))

[jzheaux]

Very much agreed that your first example is easier to read than your second.

I'd like to make sure we're on the same page with one thing that your example appears to imply.

I believe we should separate the concerns of configuring a Jwt from configuring the rest. So that means once you do:

jwt(jwt -> jwt.claim(...)...)

Then, you've completed your Jwt configuration. That configuration can propagate forward into other configurations (claims -> authorities), but not the other way around (authorities -> claims).

As such, name doesn't make sense unless it's affecting the JwtAuthenticationToken#getName method. And scopes doesn't make sense, except as a quick way to create granted authorities.

Thus, I think that we should wait on name and scopes methods for now. It's still quite easy without them:

jwt(jwt -> jwt.subject("ch4mpy"))
    .authorities("SCOPE_TEST")

or

jwt(jwt -> jwt.subject("ch4mpy").claim("scope", "TEST"))

While jwt().name("ch4mpy").scopes("TEST") is nice and quick, providing a Jwt.Builder and authorities methods is an MVP that we can iterate on in future PRs.

[ch4mpy]

1. I don't see any good reason to limit the token configuration to jwt(jwtBuilderConsumer). It would prevent users from creating method for "common JWT authentication builders" they can further modify in isolated tests (at least token would be immutable). I'm fine to provide with a shortcut to configure the token from jwt(..) and mockJwt(...) but much reserved to remove the token(...) method I wrote on JwtAuthenticationToken.Builder
2. this jwt() method name is actually aiming at configuring a JwtAuthenticationToken which is a org.springframework.security.core.Authentication and as so, also a java.security.Principal. IMO, this is enough of a reason to expose a method to configure Principal "name", specially if you want to expose methods to configure Authentication "authorities".
3. scopes(...) is just a helper to fill the gap between the spec and Spring implementation:

• "scope" is specified by https://tools.ietf.org/html/rfc6749#section-5.1 as token meta-data to authorization response
• Spring Jwt is OAuth2 but we can't access token meta-data. Adding a "scope" claim is the usual work-around.

[jzheaux]

Let's have this extend SecurityContextRequestPostProcessorSupport, following the pattern of other Spring Security request post processors that affect the SecurityContext.

[ch4mpy]

We should try and favor composition over inheritance

Inheritance seems to be required neither here nor in other request post processors. Wouldn't it be better you fill a ticket to change other post processors if you want this change to be atomic (I mean not melted in this PR)?

[jzheaux]

This is certainly worth thinking about. I think we should follow the pattern in this PR and propose changes to patterns in other PRs.

What I most appreciate about this comment is your synthesis of the various principles that inform the team's decisions. I'm not sure if I can articulate these principles in a short post here, but generally speaking, they are:

1. Follow existing patterns in the code
2. Follow good design principles, if no established pattern already exists

Since the code already has several RequestPostProcessor implementations and since JwtRequestPostProcessor fills the same kind of role as these others, it makes sense to follow the established pattern. If we were to file a ticket, it would be to change the pattern (break these out into their own files or something).

[jzheaux]

Please make the Jwt.Builder constructor private, following the pattern of other builders across the codebase and add a static factory method to Jwt called builder().

[ch4mpy]

Done

[jzheaux]

What's the use case for this method? I think it should be removed since the application can simply prepare its claims beforehand and call claims(Map).

[ch4mpy]

Nope. You requested that claims(Map) behaves the same as other builder methods for collections: add elements (not replace it). Exposing a clearClaims(Map) is the only way for a user to replace the claim set on an already configured builder.

[jzheaux]

It's atypical to use a builder as a domain object itself, i.e. with read methods. Let's please remove getClaims, hasTokenValue, etc. or make them private, if the class itself needs them.

[ch4mpy]

Done. The solution is based on extending the Jwt builder for testing purposes (provide with expected defaults)

[jzheaux]

Please remove the implements JwtClaimAccessor. Ideally, the builder will be focused on writing and the Jwt on reading.

[ch4mpy]

Done

[jzheaux]

Please add @since 5.2

[ch4mpy]

Done

[jzheaux]

Please leave this class as-is for this PR.

[jzheaux]

You can have this delegate to AuthenticationRequestPostProcessor instead of calling SecurityContextRequestPostProcessorSupport.

[jzheaux]

I think it would be better to exercise the tester's consumer at this point. Something more like:

Suggested change

	return new JwtMutator().token(jwt);
	Jwt.Builder jwtBuilder = Jwt.builder().header("alg", "none").claim(SUB, "user");
	jwtBuilderConsumer.accept(jwtBuilder);
	return new JwtMutator(jwtBuilder);

This more clearly separates the concerns between creating the Jwt and creating a JwtAuthenticationToken.

[jzheaux]

Please add some whitespace here. Thank you.

[jzheaux]

Very much agreed that your first example is easier to read than your second.

I'd like to make sure we're on the same page with one thing that your example appears to imply.

I believe we should separate the concerns of configuring a Jwt from configuring the rest. So that means once you do:

jwt(jwt -> jwt.claim(...)...)

Then, you've completed your Jwt configuration. That configuration can propagate forward into other configurations (claims -> authorities), but not the other way around (authorities -> claims).

As such, name doesn't make sense unless it's affecting the JwtAuthenticationToken#getName method. And scopes doesn't make sense, except as a quick way to create granted authorities.

Thus, I think that we should wait on name and scopes methods for now. It's still quite easy without them:

jwt(jwt -> jwt.subject("ch4mpy"))
    .authorities("SCOPE_TEST")

or

jwt(jwt -> jwt.subject("ch4mpy").claim("scope", "TEST"))

While jwt().name("ch4mpy").scopes("TEST") is nice and quick, providing a Jwt.Builder and authorities methods is an MVP that we can iterate on in future PRs.

[jzheaux]

@ch4mpy thank you very much for taking the time responding to my last review.

Let's refocus a bit. Instead of trying to address so many things at once, let's align on the scope of this ticket, and some of these other decisions will simplify after that.

Namely, let's make sure we are aligned on this comment first before moving any further. I've updated the original ticket to what I believe is an appropriate scope, given our conversation here in the PR.

In fact, it would probably be simpler for both of us to discuss the contract over on the ticket and then come back to this PR after we are in alignment there.

[jzheaux]

This is now merged into master via e59d8a5 and a polish commit of d0f5b42