initializeDatabaseConnectionVariables - Should use EnvironmentHelper as everywhere else, so retrieving environment variables is unified.

AndreasA commented 2 years ago

PHP Version

8.1

Shopware Version

6.4.70

Expected behaviour

initializeDatabaseConnectionVariables of this template should use the EnvironmentHelper utility as all others do.

Even better would be if the EnvironmentHelper is a service that could be decorated.

Actual behaviour

initializeDatabaseConnectionVariables is accessing $_SERVER etc. directly. However, the parent method does use EnvironmentHelper so this might result in different behavior.

How to reproduce

Check code: https://github.com/shopware/production/blob/78819bc811ce0a27481b7143e916305243754e87/src/Kernel.php#L29 vs https://github.com/shopware/platform/blob/v6.4.7.0/src/Core/Kernel.php#L376 and https://github.com/shopware/platform/blob/v6.4.7.0/src/Core/Kernel.php#L209

shyim commented 2 years ago

Feel free to create a PR :)

AndreasA commented 2 years ago

Done. I should now use it for all SW6 specifc variables in console and the kernel: https://github.com/shopware/production/pull/133

the symfony ones I kept as was because I think that part might just be updated to latest Symfony behavior in the future.

AndreasA commented 2 years ago

@shyim not directly related to this ticket but before I create a corresponding PR, I would like to know, if Shopware would even be open too this:

I would like to adjust the EnvironmentHelper utility to support loading values from the Symfony secrets vault. Which should be relatively easy to do, so e.g. the DATABASE_URL could be stored that way. but if you do not want this as you would need t maintaned that feature, ti does not make sense for me to put in the effort.
if not that, then as an easy solution allow environment variables used by EnvironmentHelper to be callables. That way thee get function only needs to check is_callable and if so, execute it before returning its value. This change would also be easy to maintain and give developers the most freedom.
One more feature would be to also encrypt plugin config values (especially password fields or similarly marked) using Symfony secrets when storing them in the database, to make it even more tricky for an attacker to get all the data. However, it would be enough to have corresponding event listeners e.g. a custom serializer and pre-encode / post-decode events for the value. One could also use entity loaded (or system config loader decoration) and SystemConfigChanged events but the second would be executed after the value was written and therefore require two SQL statements and having specific events for this would be easier to use.

shyim commented 2 years ago

1)

I am open to adding Symfony Secrets, but I am very sure this can be very tricky as the Kernel is not booted when the most env are used. And this should be definitively an opt-in feature. I am not sure if this feature will ever used often, as I expect better solutions for enterprise projects as any cloud provider has its own secret manager or Hashicorp Vault.

2) I would assume that the environment variables in $_SERVER are already prepared. When you want to do custom logic create a .env.local.php file. (Which is already recommended by Symfony and can be used with the Februrary Update of Shopware) and return a PHP array of your environment variables.

3) Feel free to create a PR to add such events :)

AndreasA commented 2 years ago

@shyim regarding

In a way I agree that most companies have different solutions, however, e.g. in our company we currently use 1password which does not yet provide a PHP SDK api or similar. So our current solution is to only store the symfony secrets private key in it and retrieve that one for use with Symfony secrets. might not be the perfect solution but good enough. And yes as the kernel is not loaded yet it is a bit more tricky, especially as one needs to access it without yet having access to the vault config but it is relatively simple to instantiate it with default values correctly. But yes, that is why I also agree that I am not 100% sure if it makes sense to add Symfony secrets directly as it would just be usable for a short subset and think some generic events might be better.
As far as I understand the .env.local.php file is usually dumped automatically by a Symfony command, so its output should not be modified. Also for it to be dumped (easily), symfony/flex has to be installed as well but I guess I could mis-use it by putting loadEnv into it and then doing custom stuff but I would prefer not to do so. One way would be to allow some value transformers in the EnviornmentHelper class like e.g. EnviornmentHelper::registerTransformer($variable, $transformer), files could then be loaded using composer autoload/files section to register a corresponding transformer. Or similar? If so, I would create a PR for it. It would basically allow closures to be added. In a normal Symfony setup this wouldn't be an issue because doctrine dbal is initialized using Symfony config, so secrets can be injected into its config directly, However, as Shopware needs the DB connection quite early that is not posible.

AndreasA commented 2 years ago

@shyim If you would be happy with EnviornmentHelper::registerTransformer($variable, $transformer) solution I would try to create a PR this week and also one for the suggested events in 3.

AndreasA commented 2 years ago

@shyim So I would modify the EnviornmentHelper to something like this - if that is fine I will create a PR for the platform:

<?php declare(strict_types=1);

namespace Shopware\Core\DevOps\Environment;

class EnvironmentHelper
{
    private static array $transformers = [];

    /**
     * Reads an env var first from $_SERVER then from $_ENV super globals
     * The caller needs to take care of casting the return value to the appropriate type
     *
     * @param bool|float|int|string|null $default
     *
     * @return bool|float|int|string|null
     */
    public static function getVariable(string $key, $default = null)
    {
        $value = $_SERVER[$key] ?? $_ENV[$key] ?? $default;

        $transformers = self::$transformers[$key] ?? [];

        foreach ($transformers as $transformer) {
            $value = $transformer($key, $value);
        }

        return $value;
    }

    public static function hasVariable(string $key): bool
    {
        return \array_key_exists($key, $_SERVER) || \array_key_exists($key, $_ENV);
    }

    public static function registerTransformer(string $key, string $name, \Closure $closure): void
    {
        self::$transformers[$key][$name] = $closure;
    }
}

maybe intead of clousre an interface though.

shyim commented 2 years ago

Looks good for me

AndreasA commented 2 years ago

@shyim Great. Will create a PR. Hopefully sometime this week.

shopware / production