Deprecated WithMockJwtAuth Annotation Concern about spring-addons HOT 12 OPEN

You can set only very partial JSON as input to @WithJwt (just the claims you need, not necessarily a "full" token). It's main advantage is it takes the authentication converter from the application context, so no surprise at runtime with the way this claims are turned into authorities. As a contrast, authorities with @WithMockJwtAuth are purely declarative and de-correlated from the claims.

The reason why @WithMockJwtAuth wasn't removed is because it offers this explicit claims declaration just above the test body. The reason why it is deprecated is because it makes it simple to have claims not really coherent with the authentication authorities and name. There is no plan to remove it. Maybe if I see that there is still enough interest in it (like votes on this issue), I'll try to find a way to use the authentication converter for it too (and remove this deprecation).

Using this annotations quite a lot on "real" projects (most with some UX approach at some point), it turned out that having a few personas and specifying access controls rules with them gave a much better grasp of what was happening to business people (when talking about fine grained authorities for each endpoint, they were frequently lost). So having a JSON file for each persona and decorating the test with its name was actually helping a lot in reviews to validate that test context is actually what the business expects.

from spring-addons.

Thanks for the explanation. I understand the concern about the possible impedance mismatch between test security claims and real-world production claims. I'm thinking right now how I would go about crafting a test case that shows this in a concrete form. This would help me think through the pros and cons of this issue some more.

Unfortunately, due to work time constraints, I can't actually spend much time testing this out right now. I might be able to in a week or two.

Luckily (!) for me, I don't have management that care about personas or anyone else really that I need to have these types of discussions with about security related code. Mostly, their eyes glaze over if I start talking about token claims. For better or worse.

If this annotation isn't going to be removed, is the deprecation just there so that people will look at the note? If that is the case, could you put more comments on the deprecation indicating that it won't be going away and why? Maybe if I get around to making the demo test cases, we could write up some better documentation explaining the case a little bit better to people.

from spring-addons.

@danielshiplett I investigated a bit around @WithMockJwtAuth and @WithMockBearerTokenAuthentication.

It wasn't complicated to search for the authentication converter in the application context and to inject it in the authentication factory behind each annotation.

This improves the situation a bit: the authentication type is the right one when such a converter is provided and returns something else than the default Authentication implementation. However, this annotations are way more complicated to use in JUnit 5 @ParameterizedTest (compared to @WithJwt and @WithOpaqueToken).

As consequence, I'll keep it deprecated for now. Maybe, if I find a way to have it well integrated with @ParameterizedTest, I'll remove the deprecation, but for now, I'm too busy on other subjects to investigate more.

from spring-addons.

@ch4mpy Need your help with the latest version of the library. I could not get our tests working after upgrading to 7.3.0.

Current working tests -> https://github.com/eclipse-xpanse/xpanse/blob/main/runtime/src/test/java/org/eclipse/xpanse/runtime/AdminServicesApiWithZitadelAndMysqlTest.java#L86
when we move the authorities to json file, I always get 403.

i tried to set the authorities at different places. But does not help.

{
  "preferred_username": "xpanse-user",
  "email": "[email protected]",
  "aud": "https://localhost:7082",
  "scope": "openid user isv admin",
  "roles": ["user", "isv", "admin"],
  "urn:zitadel:iam:org:project:roles": {
    "user": "user",
    "isv": "isv"
  },
  "sub": "1234566"
}

Could you please let me understand what am I missing?

Regards,
Swaroop

from spring-addons.

Is your authentication converter exposed as a @Bean?

Can you please add it here?

The authentication converter is a Converter<Jwt, ? extends AbstractAuthenticationToken> that you might provide in resource server configuration as JwtAuthenticarionConverter.

from spring-addons.

An option to have your test pass is using @WithMockAuthentication instead of @WithMockJwtAuth. This annotation is fine when you don't care about the authentication type (but is rather limited about what you can configure on the test Authentication instance).

@swaroopar from what I can see in your repo, you configure no SecurityFilterChain bean, which means that you are relying entirely of the defaults of the spring-boot starters you are using.

Regarding security in your pom, all I could find is:

        <dependency>
            <groupId>org.eclipse.xpanse.modules</groupId>
            <artifactId>security</artifactId>
            <version>${project.version}</version>
        </dependency>

Unfortunately, I could not find any useful documentation about what the Spring Boot starter behind it configures and I don't really feel like digging into their source code.

What I can see in your application-zitadel.properties is pretty confusing: are you using token introspection, JWT decoding or Zitadel proprietary adaptor? This is important because it will determine the content of the default SecurityFilterChain built by your Boot starter and the type of Authentication you get at runtime.

The annotation to use in your test depending on the actual type of Authentication you get at runtime, you probably need a better insight of what you are configuring with all the starters you use.

An option to consider would be using spring-addons-starter-oidc instead of xpanse security and Zitadel adapters. spring-addons can adapt to any OIDC Provider (including Zitadel) and probably better documents what it configures...

from spring-addons.

@ch4mpy thank you for taking time for my question.

we have a filter bean here -> https://github.com/eclipse-xpanse/xpanse/blob/main/modules/security/src/main/java/org/eclipse/xpanse/modules/security/zitadel/config/ZitadelWebSecurityConfig.java#L96
and we have a JWTDecoder and converter -> https://github.com/eclipse-xpanse/xpanse/blob/main/modules/security/src/main/java/org/eclipse/xpanse/modules/security/zitadel/config/ZitadelWebSecurityConfig.java#L178

could you please see if this information helps? Would still first like to see if I can get the tests working with WithMockJwtAuth or even better if with the recommmended @WithJwt annotation.

P.S - We will also check and consider switching to spring-addons-starter-oidc in the coming days.

from spring-addons.

@swaroopar if you want @WithJwt to work as expected, you'll have to expose what is currently called grantedAuthoritiesExtractor() in your conf as a bean (and inject it in your security filter-chain).

Your conf is unusual: switch between access token introspection and JWT decoding depending on an application property.

Switching from one to another has major impact at runtime: the default Authentication type in the security context is not the same, and the way this authentication is built is pretty different. In your case, you'll get

• for introspection what the OpaqueTokenAuthenticationConverter builds. You are using the default, so that will be a BearerTokenAuthentication.
• for JWT decoding, what your grantedAuthoritiesExtractor() returns (this method would be much better named jwtAuthenticationConverter()): JwtAuthenticationToken

Also note that you should probably be sharing the authorities extraction logic between the introspection and JWT decoding (see in the example conf below how).

The test annotation to use is not the same in the different cases:

• @WithJwt (and now @WithMockJwtAuth) use the Converter<Jwt, AbstractAuthenticationToken> in the context and are adapted to apps with JWT decoder
• @WithOpaqueToken use OpaqueTokenAuthenticationConverter in the context and is adapted to token introspection

What you could do in your case is a unified Authentication impl for introspection and JWT decoding:

@Data
@RequiredArgsConstructor
public static class SimplePrincipal implements Principal {
	private final String name;
}

public static class XpanseAuthentication extends AbstractAuthenticationToken {
    private static final long serialVersionUID = -2797266050799781972L;

    private final Map<String, Object> claims;
    private final Principal principal;
    private final String token;

    public XpanseAuthentication(String username, Collection<? extends GrantedAuthority> authorities, Map<String, Object> claims, String token) {
        super(authorities);
        this.claims = Collections.unmodifiableMap(claims);
        this.principal = new SimplePrincipal(username);
        this.token = token;
        super.setAuthenticated(true);
    }

    @Override
    public Map<String, Object> getCredentials() {
        return claims;
    }

    @Override
    public Principal getPrincipal() {
        return principal;
    }

    public String getTokenString() {
        return token;
    }
}

@Slf4j
@Component
static class GrantedAuthoritiesExtractor implements Converter<Map<String, Object>, Collection<GrantedAuthority>> {

    @Override
    public Collection<GrantedAuthority> convert(Map<String, Object> claims) {
        Set<GrantedAuthority> roles;
        String userId = claims.get(USERID_KEY);
        Map<String, Object> rolesClaim = claims.get(GRANTED_ROLES_KEY);
        if (Objects.isNull(rolesClaim) || rolesClaim.isEmpty()) {
            roles = Set.of(new SimpleGrantedAuthority(DEFAULT_ROLE));
            log.info("Get user [id:{}] granted authorities is empty," + " set default authority user", userId);
        } else {
            roles = rolesClaim.keySet().stream().map(SimpleGrantedAuthority::new).collect(Collectors.toSet());
            log.info("Get user [id:{}] granted authorities:{}.", userId, roles);
        }
        return roles;
    }
}

static interface XpanseAuthenticationConverter extends Converter<Jwt, XpanseAuthentication> {
}

@Bean
// This Converter<Jwt, AbstractAuthenticationToken> must be exposed as a bean to be picked by @WithJwt
XpanseAuthenticationConverter jwtAuthenticationConverter(Converter<Map<String, Object>, Collection<GrantedAuthority>> authoritiesConverter) {
    return jwt -> {
        final var username = (String) jwt.getClaims().get(USERID_KEY);
        final var authorities = authoritiesConverter.convert(jwt.getClaims());
        return new XpanseAuthentication(username, authorities, jwt.getClaims(), jwt.getTokenValue());
    };
}

@Bean
// This OpaqueTokenAuthenticationConverter must be exposed as a bean to be picked by @WithOpaqueToken
OpaqueTokenAuthenticationConverter opaqueTokenAuthenticationConverter(Converter<Map<String, Object>, Collection<GrantedAuthority>> authoritiesConverter) {
    return (String introspectedToken, OAuth2AuthenticatedPrincipal authenticatedPrincipal) -> {
        final var username = (String) authenticatedPrincipal.getAttributes().get(USERID_KEY);
        final var authorities = authoritiesConverter.convert(authenticatedPrincipal.getAttributes());
        return new XpanseAuthentication(username, authorities, authenticatedPrincipal.getAttributes(), introspectedToken);
    };
}

@Bean
SecurityFilterChain apiFilterChain(
        HttpSecurity http,
        HandlerMappingIntrospector introspector,
        Converter<Jwt, AbstractAuthenticationToken> jwtAuthenticationConverter,
        OpaqueTokenAuthenticationConverter opaqueTokenAuthenticationConverter) {
    ...

    if (StringUtils.equalsIgnoreCase(AUTH_TYPE_TOKEN, authTokenType)) {
        // Config custom OpaqueTokenIntrospector
        http
                .oauth2ResourceServer(
                        oauth2 -> oauth2
                                .opaqueToken(
                                        opaque -> opaque
                                                .introspector(new ZitadelOpaqueTokenIntrospector(introspectionUri, clientId, clientSecret))
                                                .authenticationConverter(opaqueTokenAuthenticationConverter)));
    }

    if (StringUtils.equalsIgnoreCase(AUTH_TYPE_JWT, authTokenType)) {
        // Config custom JwtAuthenticationConverter
        http.oauth2ResourceServer(oauth2 -> oauth2.jwt(jwt -> jwt.jwtAuthenticationConverter(jwtAuthenticationConverter)));
    }
    return http.build();
}

This will bring some homogeneity at runtime, but you'll still have to test separately your app for introspection and JWT decoding (with dedicated annotation for each).

Spring-addons contains such an Authentication implementation as an opt-in: OAuthentication<OpeindClaimSet> (defaults are left to JwtAuthenticationToken and BearerTokenAuthentication)

Again, for your current tests, @WithMockAuthentication is just enough. Also, what you is initiated in the xpanse security module is probably more mature in spring-addons to quite some regards (and less adherent to a single OIDC Provider)

from spring-addons.

@ch4mpy thank you again for the detailed answer. Your feedback makes complete sense to me. For now, I have first got the tests working with @WithMockAuthentication. Will now check and update the other changes which you have suggested.

P.S - regarding an option to switch between opaqueToken and JWT - we did this because we earlier had only opaqueToken and then found out that the OpaqueToken process increases the load on the Oauth system. So we introduced JWT and then just to keep it backward compatible, we also allowed OpaqueToken to be used by consumers if needed.

from spring-addons.

@swaroopar I'm waiting for your questions about spring-addons and how it could match your needs.

By the way, regarding the way you switch between JWT decoding and introspection, you are not quite following the Spring Boot way of doing things. What I do for that in spring-addons is exposing @ConditionalOnProperty beans and the user provides either JWT decoding OR introspection one (just like he does in vanilla Spring Boot projects). The framework then look what is on the classpath (properties and jars) and builds a coherent configuration out of that (or throws an error when there are inconsistencies)

from spring-addons.

@ch4mpy thank you for asking. It is on my TODO list and hopefully will find sometime this week to refactor it.

Regarding the switch between JWT and introspection, we are currently using the property directly to configure it. Will definitely consider your suggestion during refactoring.

I will get back to you when it is done.

from spring-addons.

@ch4mpy I refactored our security related code as per suggestion and now the test works with @WithJwt as well. Thank you again 🙏

from spring-addons.