DuendeSoftware / Support

Support for Duende Software products
21 stars 0 forks source link

How to decorate ITokenRequestValidator? #1277

Closed GMillerVarian closed 3 months ago

GMillerVarian commented 4 months ago

Which version of Duende IdentityServer are you using? 7.0.4

Which version of .NET are you using? 8.0

I am having trouble decorating ITokenRequestValidator as described in #951. The problem is that the default TokenRequestValidator is internal, so I cannot register it with DI, nor inject it into my custom validator. I have also tried using reflection, and while the code compiles it just hangs when requesting a token (I suspect a circular dependency in DI since my class both implements ITokenRequestValidator and takes ITokenRequestValidator as a parameter).

@AndersAbel I don't understand the following statement: "Then you can add your own registration for ITokenRequestValidator that takes in InnerWrapper<ITokenRequestValidator> as a parameter." What is InnerWrapper?

Could you provide a simple example showing how to hook this all up?

Here is my custom class:


public class MyTokenRequestValidator(ITokenRequestValidator defaultTokenRequestValidator) : ITokenRequestValidator
{
    private readonly ITokenRequestValidator _defaultTokenRequestValidator = defaultTokenRequestValidator;

    public async Task<TokenRequestValidationResult> ValidateRequestAsync(TokenRequestValidationContext context)
    {
        var validationResult = await _defaultTokenRequestValidator.ValidateRequestAsync(context);
        if (validationResult.IsError)
        {
            return validationResult;
        }

        var grantType = context.RequestParameters.Get(TokenRequest.GrantType);
        if (grantType == GrantTypes.ClientCredentials)
        {
            // Always require client secret for ClientCredentials auth flow
            validationResult.ValidatedRequest.Client.RequireClientSecret = true;
        }

        return validationResult;
    }
}
AndersAbel commented 4 months ago

We use this pattern internally in IdentityServer, the implementation is here.

If you just want to decorate an implementation with a known registration type you can make a more simple implementation. The type that I named InnerWrapper<T> in the linked question is called Decorator<TService, TImpl> in the linked code.

GMillerVarian commented 4 months ago

Thank you, I was able to get the custom TokenRequestValidator hooked up.

However, stepping through I see the ClientSecretValidator is called before the TokenRequestValidator, which means setting RequireClientSecret=true in TokenRequestValidator is too late.

Looking at the ClientSecretValidator code, it seems much more difficult to customize.

Can you offer any suggestions on how we could enforce RequireClientSecret=true for all client_credentials token requests?

AndersAbel commented 3 months ago

I'm sorry I didn't read at first what you wanted to do, just the how part. That's why I focused on the decorator.

To add extra validation rules on the clients, override the IClientConfigurationValidator service. You can derive from DefaultClientConfigurationValidator and override ValidateSecretsAsync to add an extra check.

GMillerVarian commented 3 months ago

The complication here is that validation needs to be done in the context of a token request.

For example, if we have a single client registered with both authorization_code and client_credentials grant types, and requireClientSecret=false.

When requesting a token using authorization_code flow, it's fine - no client secret is required. However, with client_credential flow, we need to set requireClientSecret=true so we fail if no client secret was supplied.

I realize that ideally these should be 2 separate clients, and we would validate against this during client registration - but we have consumers already using this configuration and we cannot easily (or at least quickly) make this change as it would be a breaking change.

brockallen commented 3 months ago

The other hook would be a custom token request validator: https://docs.duendesoftware.com/identityserver/v7/reference/validators/custom_token_request_validator/

AndersAbel commented 3 months ago

I see, then it's a bit more complicated.

First of all, I would like to point out that a single client id should be used for a single client. If that client is a public client without a client secret it should not be allowed to use the client credential flow at all. If the client is a confidential (server-based) client it should have a client secret. Then it would be fine to allow it to use the client credential flow.

If you have one public client application (i.e. a mobile or native app) and a server based component they should, as you say, be given two different client ids. But I also know that the perfect architecture doesn't exist in the computer industry. Sometimes we need to fix the situation we are in, because it's not possible to go back and change some decisions/settings.

The right way to do this validation would be to register an ICustomTokenRequestValidator and do the validation in ValidateAsync:

if (context.Result!.ValidatedRequest.GrantType == GrantType.ClientCredentials
    && !context.Result!.ValidatedRequest.Client.RequireClientSecret)
{
    // Client secret have not been validated as it is flagged as not required.
    // Use the ISecretsListValidator to do that.

    var validationResult = await secretsListValidator.ValidateAsync(
        context.Result!.ValidatedRequest.Client.ClientSecrets,
        context.Result!.ValidatedRequest.Secret!);

    if (!validationResult.Success)
    {
        context.Result.IsError = true; 
        context.Result.Error = IdentityModel.OidcConstants.TokenErrors.InvalidRequest;
    }
}
GMillerVarian commented 3 months ago

Thank you very much for this suggestion, we will reach out again if any issues.