Multitenancy tokenvalidation example should be adjusted

[daniKir]

Hi everyone,

First of all I am very grateful for the good examples in your documentation! I recently however stumbled across this example about multitenancy which in my mind has some major problems:

spring-security/docs/modules/ROOT/pages/servlet/oauth2/resource-server/multitenancy.adoc

Lines 375 to 400 in 8cdd53f

	@Component
	public class TenantJwtIssuerValidator implements OAuth2TokenValidator<Jwt> {
	private final TenantRepository tenants;
	private final Map<String, JwtIssuerValidator> validators = new ConcurrentHashMap<>();
	public TenantJwtIssuerValidator(TenantRepository tenants) {
	this.tenants = tenants;
	}
	@Override
	public OAuth2TokenValidatorResult validate(Jwt token) {
	return this.validators.computeIfAbsent(toTenant(token), this::fromTenant)
	.validate(token);
	}
	private String toTenant(Jwt jwt) {
	return jwt.getIssuer();
	}
	private JwtIssuerValidator fromTenant(String tenant) {
	return Optional.ofNullable(this.tenants.findById(tenant))
	.map(t -> t.getAttribute("issuer"))
	.map(JwtIssuerValidator::new)
	.orElseThrow(() -> new IllegalArgumentException("unknown tenant"));
	}
	}

First of all the code is rather complex for a very simple logic. If a incoming token is validated, there are only 3 possible branches in the code:

1. The issuer is already in the map. In this case the according JWTIssuerValidator is called which will always yield a successfull validation, as all issuers in the map are only ever called by their map-key.
2. The issuer is not in the map and not know by the TenantRepository. In this case we will always get an IllegalArgumentException.
3. The issuer is not in the map and known by the TenantRepository. In this case we will add a JWTIssuerValidator to the map, which will always be successfull.

This behaviour could be achieved by using a whitelist of known issuers and simply expanding is when a new valid issuer is found:

@Override
public OAuth2TokenValidatorResult validate(Jwt token) {
    String tenant = toTenant(token);
    if(allowedTenants.contains(tenant)) {
        return OAuth2TokenValidatorResult.success();
    }
    if(this.tenants.findById(tenant) != null) {
        allowedTenants.add(tenant);
        return OAuth2TokenValidatorResult.success();
    }
    throw new IllegalArgumentException("unknown tenant");
}

This however shows a few more deficits of the design:

1. The map seems to be designed as a cache/lazy-loading mechanism. It however only caches successfull results, and negative results will never be cached.
2. If a tenant is ever removed from TenantRepository and is already cached it will not be removed from the whitelist.
3. The IllegalArgumentException seems to contradict the method contract, I feel it would be more appropriate to return a OAuth2TokenValidatorResult.failure().

I would therefore suggest this more flexible and simpler design similar to JwtClaimValidator by spring-security:

@Component
public class TenantJwtIssuerValidator implements OAuth2TokenValidator<Jwt> {
    private final TenantRepository tenants;

    private final OAuth2Error error = new OAuth2Error(OAuth2ErrorCodes.INVALID_TOKEN, "The iss claim is not valid",
            "https://tools.ietf.org/html/rfc6750#section-3.1");

    public TenantJwtIssuerValidator(TenantRepository tenants) {
        this.tenants = tenants;
    }

    @Override
    public OAuth2TokenValidatorResult validate(Jwt token) {
        if(this.tenants.findById(token.getIssuer()) != null) {
            return OAuth2TokenValidatorResult.success();
        }
        return OAuth2TokenValidatorResult.failure(this.error);
    }
}

or even

@Component
public class TenantJwtIssuerValidator implements OAuth2TokenValidator<Jwt> {

    private final JwtClaimValidator<String> jwtIssuerValidator;

    public TenantJwtIssuerValidator(TenantRepository tenants) {
        this.jwtIssuerValidator = new JwtClaimValidator<>(JwtClaimNames.ISS, issuerClaim -> this.tenants.findById(issuerClaim) != null);
    }

    @Override
    public OAuth2TokenValidatorResult validate(Jwt token) {
        return jwtIssuerValidator.validate(token);
    }
}

If one is willing to adjust the TenantRepository api one could of course make this look even better by using a method reference.
If there is no need to support dynamically changing properties one could of course also build a complete whitelist at bean creation.
The kotlin example should be adjusted accordingly.

[jzheaux]

Hi, @daniKir, thanks for the suggestion.

I agree that this code sample likely tries to address too many of the reader's concerns at once. They can certainly add caching, etc. in a way that makes sense for their application without suggestions from Spring Security.

Are you able to submit a PR to update the documentation? And if so, would you please make the change on the 5.8.x branch? Then when it gets merged, we can easily make sure the update makes it to all branches.

[daniKir]

Hi @jzheaux, thank you for you response. I will gladly submit a PR with a documentation update. Do you have any preferences on the (last two) solutions I suggested?

[jzheaux]

Great! I'd do the first of the two. I feel like it's a bit easier to read, which is valuable in documentation.