Allow additional public keys to be set by value, as well as by path

[MatthewHepburn]

#928 added an excellent feature, allowing additional public keys to be specified.

However, unlike the main public key, this can only be specified as a file path, rather than being passed directly as a string. The ability to pass keys as strings is useful when setting keys via environment variables, so I want to be able to configure the additional public keys as either file paths or as raw strings.

[chalasr]

Sounds good! Let me know when you think this is ready

[chalasr]

what's the motivation for this change?

[MatthewHepburn]

I want to support having a dynamic number of additional keys using a static configuration. For example 1 additional key while a key rotation is in progress, and 0 additional keys otherwise.

I haven't been able to come up with a better way of expressing that than

lexik_jwt_authentication:
  public_key: '%env(MAIN_KEY)%'
  additional_public_keys:
    - '%env(string:default::SECONDARY_KEY)%'

But then we end up with this invalid array element when SECONDARY_KEY is not set that needs to be skipped for that to work.

I did consider trying to set the whole array via JSON like

lexik_jwt_authentication:
  public_key: '%env(MAIN_KEY)%'
  additional_public_keys: '%env(json:SECONDARY_KEY_JSON)%'

But that's not allowed - we get the error A dynamic value is not compatible with a "Symfony\Component\Config\Definition\PrototypedArrayNode" node type at path "lexik_jwt_authentication.additional_public_keys".

I have limited experience with the Symfony config system, so perhaps I'm missing a better way of expressing this. I would be happier keeping the behaviour of throwing on encountering a falsy value and handling this situation at the config level instead, but I'm not seeing a way to do that.

[chalasr]

Let me think about it :)

[1ed]

We also have a similar use case.

[danieleartico]

Hello! I'm facing a similar problem: using additional string certs instead of files to verify tokens.

IMHO, looking at the code, the original intention of the AbstractKeyLoader was to allow both cases because the $key is expected to be stored in the $contents array in its raw format even if the file is missing. Furthermore a similar behavior is defined in the getSigningKey() method.

Unfortunately, due to the (wrong?) if statement at the beginning of the loop, this situation never happens because when the key is properly set but it does not point to a file, then the exception is thrown.

@MatthewHepburn beware that what you tried to achieve in your first commit did not solve this problem because, when the $key is properly configured, !$key always solves to false and the condition on the right side is never considered 🙂

Also, I'd leave the \RuntimeException if the string is empty, because in my opinion it is a config-related issue and it should not be addressed here 🤔

That said, I'd change the code as following:

if (!$key || (is_file($key) && !is_readable($key)) {
    throw new \RuntimeException(...);
}

What do you think?

[Ardenexal]

@chalasr @MatthewHepburn any update on this I running into the same issue where I want to pass through the Public key string rather than the file location and this PR would fix that issue

[1ed]

What is missing to get this merged?

[TekSign]

Is there any update on this change please ? Because this would be very helpful and all seems ready.