mollie / laravel-mollie

Mollie API client wrapper for Laravel & Mollie Connect provider for Laravel Socialite
https://www.mollie.com/
MIT License
322 stars 62 forks source link

Enhancement Request: Support for Dynamic API Key Configuration via Closures in Laravel Package #245

Closed npostma closed 2 months ago

npostma commented 2 months ago

Expected Behavior

The ability to set the API key using a config closure or another method without relying on the .env file or cached configuration. This enhancement will make the package suitable for use in a SaaS solution. Due to Laravel's caching mechanisms, using Config::set() is not recommended.

Current Behavior

Currently, only one static API key can be set, which is not safe for runtime configurations.

Possible Solution

Support for closures in the configuration would be the easiest way to implement this:

return [

    'key' => function () {
        // Assuming you have a Setting model where you store your settings
        return \App\Models\Setting::where('key', 'mollie_api_key')->value('value');
    },

    // If you intend on using Mollie Connect, place the following in the 'config/services.php'
    // 'mollie' => [
    //     'client_id'     => env('MOLLIE_CLIENT_ID', 'app_xxx'),
    //     'client_secret' => env('MOLLIE_CLIENT_SECRET'),
    //     'redirect'      => env('MOLLIE_REDIRECT_URI'),
    // ],

];

Steps to Reproduce

Attempt to set the API key dynamically at runtime. Observe that the current implementation only allows setting a static key through the .env file or cached configuration.

Context (Environment)

Laravel version: 10.X.X PHP version: All Package version: Latest

Additional Information

Supporting closures in the configuration will allow for more flexible and secure management of API keys, especially in multi-tenant SaaS applications. This approach ensures that the API key can be retrieved from a database or another secure source at runtime, enhancing security and adaptability. To implement this feature, users must elevate (publish) the vendor's configuration file to their own config directory using the php artisan vendor:publish command. This will allow them to modify the configuration to support dynamic API key retrieval.

npostma commented 2 months ago

See pullrequest.

Naoray commented 2 months ago

Hi @npostma,

Thanks for the idea. Using closures in config files prevents caching. Therefore I will close the PR.

You can reach the same behavior by extending the MollieApiClient from the container and inserting the setting value.

$this->app->extend(MollieApiClient::class, function (MollieApiClient $client) {
    $client->setApiKey(
        \App\Models\Setting::where('key', 'mollie_api_key')->value('value')
    );

    return $client;
});

If you have further questions, feel free to ask them here.

npostma commented 2 months ago

Using closures in config files prevents caching. That is the whole idea. In multi tenant environments you can't have config caching because you will have the risk of data leaks. Is your suggestion safe to use, without the change of a data leak?

Naoray commented 2 months ago

I've worked on multi-tenant apps and we've always cached our configs. Can you explain the "risk of data leaks" concern? Also, I'm unclear about your question regarding the safety of my suggestion. My proposal involves reading the API key from the Settings model at runtime.

npostma commented 2 months ago

Sure, from what I understand, Laravel caches configurations at the application scope. In the multi-tenant application I am currently working on, has an organization scope within the application scope. I want to be absolutely sure that when a payment is fulfilled, the cache key for that specific organization is used and not a random cached key, which could potentially belong to another organization.

Extending the framework is new to me in a way that, I am not fully aware of its internal workings. If this process is executed once and then cached, subsequent calls would retrieve the data from the cache instead of the database. In a multi-tenant application, this could potentially cause issues if the cache keys are not managed correctly. Specifically, there's a risk of retrieving data meant for one organization under a different organization's context. To mitigate this, it is crucial for me to ensure that cache keys are unique to each organization.

[EDIT] Upon further investigation, i think your solution would also work just fine. Adding that sniped to a Custom MollieServiceProvider's register function. Thanks for the info.

Naoray commented 2 months ago

Sure, from what I understand, Laravel caches configurations at the application scope. In the multi-tenant application I am currently working on, has an organization scope within the application scope

Your configurations are always directly related to your whole application and should not be tinkered with on runtime. The multi tenant specific auth details should be read in runtime and fed into the services that require this information. The config is not meant to be used this way.

Extending the framework is new to me in a way that, I am not fully aware of its internal workings. If this process is executed once and then cached, subsequent calls would retrieve the data from the cache instead of the database.

In PHP, each request starts a new instance of the application. The entire application lifecycle, including service providers and bindings, is re-executed for each request. Laravel’s container bindings are not cached across requests, meaning the bindings and service providers are resolved fresh each time.

I am closing this for now. If you have further issues please feel free to reopen this ticket

sandervanhooft commented 2 months ago

Also note that from a security perspective oauth is highly recommended for multi-tenant setups.