Is it bad practice to instantiate an IConfidentialClientApplication within a class?

Question

As an example, I have an AuthorizationClient registered as a transient service in my application configuration that acts as a client mediator for authorization tokens between my API and Azure. Inside, there is an IConfidentialClientApplication object property:

public class AuthorizationClient : IAuthorizationClient
{
    private readonly string[] _resourceIds;
    private IConfidentialClientApplication App;

    public AuthorizationClient(IAuthenticationConfig AuthenticationConfig, IConfiguration configuration)
    {
        var scope = "/.default";
        var resourceId = "api://" + configuration[AuthenticationConfig.ResourceID] + scope;
        var clientId = configuration[AuthenticationConfig.ClientID];
        var clientSecret = configuration[AuthenticationConfig.ClientSecret];            
        var instance = AuthenticationConfig.Instance;
        var tenant = configuration[AuthenticationConfig.AzureTenantId];        

        var authority = string.Format(CultureInfo.InvariantCulture, instance, tenant);
            
        _resourceIds = new string[] { resourceId };
        try
        {
            App = ConfidentialClientApplicationBuilder.Create(clientId)
            .WithClientSecret(clientSecret)
            .WithAuthority(new Uri(authority))                
            .Build();
        }
        catch(Exception ex)
        {
            //TODO logger
        }

    }
...
}

Elsewhere in this class, I have methods that successfully fetch the token, check token cache, etc.

But when I've seen documentation or repositories of IConfidentialClientApplication in use, it's always added as a service at the application level, not inside another object. Is there potential pitfalls with the way I've done this?

The IConfidentialClientApplication seems rather heavy/large if a new one is instantiated per every client request, but as I've instantiated it inside an object that is an app-level service, only one seems to be created per lifecycle.

Answer 1

Instantiating dependencies within your target class is a code smell

Pitfalls: Tight Coupling to implementation details and Explicit Dependency Principle violation.

these will make maintaining and testing your class in isolation difficult.

These design issues can be fixed to make your code more SOLID by applying Single Responsibility Principle / Separation of Concerns and Explicit Dependency Principle

Create a class to store client options. Based on the provided example it may look like this

public class AuthorizationClientOptions {
    public string[] ResourceIds { get; set; }
}

Refactor the class to explicitly depend on what it actually needs to perform its function

public class AuthorizationClient : IAuthorizationClient {
    private readonly string[] _resourceIds;
    private readonly IConfidentialClientApplication app;

    public AuthorizationClient(IConfidentialClientApplication app, IOptions<AuthorizationClientOptions> options) {
        this.app = app;
        _resourceIds = options.Value.ResourceIds;
    }
    
    // ...
}

Configure the necessary implementation details in the composition root (Startup)

//...

IConfiguration Configuration; //Populated either via constructor injection or manually 

public void ConfigureServices(IServiceCollection services) {

    //...
    
    //Configure options
    services.AddOptions<AuthorizationClientOptions>()
        .Configure<IAuthenticationConfig,IConfiguration>(
            (o, authConfig, config) => {
                var scope = "/.default";
                var resourceId = "api://" + config[authConfig.ResourceID] + scope;
                o.ResourceIds = new string[] { resourceId };
            });
            
    //Configure dependency using factory delegate
    services.AddSingleton<IConfidentialClientApplication>(sp => {
        IAuthenticationConfig AuthenticationConfig = sp.GetRequiredService<IAuthenticationConfig>();
        
        var instance = Configuration.Instance;
        var tenant = Configuration[AuthenticationConfig.AzureTenantId];
        var authority = string.Format(CultureInfo.InvariantCulture, instance, tenant);
        
        var clientId = Configuration[AuthenticationConfig.ClientID];
        var clientSecret = Configuration[AuthenticationConfig.ClientSecret];
        
        return ConfidentialClientApplicationBuilder.Create(clientId)
            .WithClientSecret(clientSecret)
            .WithAuthority(new Uri(authority))
            .Build();
    });
    
    services.AddScoped<IAuthorizationClient, AuthorizationClient>();
    
    //...
}

The IConfidentialClientApplication seems rather heavy/large if a new one is instantiated per every client request

The IConfidentialClientApplication in the above example is created as a singleton, so it will have a one-time cost when instantiated.