"token_ttl" must be a numeric value, but that's not compatible with Sf 3.2+ %env(FOO)% mechanism

[olivierphi]

Hi,

As we now have this shiny %env(FOO)% mechanism in Symfony configuration for a few months, and as it looks like it will be omnipresent in Symfony 4, it would be nice to be able to use it with this (wonderful) Bundle, wouldn't it?

But if I'm not wrong, it's not possible at the moment, because of this check in the Bundle's semantic configuration.

From what I understood from that Symfony PR, these %env(FOO)% placeholders are actually replaced with placeholders (which are strings) at container compilation time, and will be replaced with the matching environment variables values at runtime.

Because of this, with the following config:

lexik_jwt_authentication:
  token_ttl: '%env(JWT_TOKEN_TTL)%'

...I get the following at container build time:

Hence the validation fail.

Maybe it would be worth removing that is_numeric() check, so that we could actually configure our tokens TTL via environment variables?
If you're ok with that, I should be able to submit a PR soon.

Thanks!

[chalasr]

Hi @drbenton,

You should have a look at the is_numeric function in the manual http://php.net/manual/en/function.is-numeric.php.
The whole point of using is_numeric is to handle numeric strings, e.g:

is_numeric('42') // true

So using environment variables for the ttl is actually fine regarding config validation.
However, your issue made me realize that this bundle is not yet compatible with runtime environment variables (%env()% syntax), because an internal parameter is created from each value of the bundle configuration, meaning that the value is dumped within the container thus not dynamic at all.

I will take care of fixing this very quickly. Thank you for pointing it out.
Please let this open until it is fully supported.

[olivierphi]

Hi @chalasr

Indeed, this is how is_numeric works, but as you said the issue here is that in the case of an %env()% syntax we receive a value which doesn't look like a numeric value at all - hence the failure of is_numeric, even if alter on we do provide a numeric-ish value at runtime via en environment var :-)

Thank you very much for taking care of it!

Olivier

[chalasr]

@drbenton I'm not sure to get what you mean. Why wouldn't the env var be a numeric value? Could you please give an example of the value that could reach this statement in your case?

[chalasr]

When using %env()% (or just %%), the parameter values are resolved with the whole configuration before being processed (thus before reaching this statement), which also make actually impossible to have them dynamic at runtime.

[olivierphi]

Yeah, because at build time we receive a "dynamic parameter placeholder" rather than a real value.

As you can see on my XDebug session screenshot, even if I do have a JWT_TOKEN_TTL=86400 env var I receive a env_JWT_TOKEN_TTL_[some md5] string placeholder at build time. This is done automatically by the DIC when it detects the %env()% syntax.
So which will never look like a numeric value, even if the value behind it is numeric.
At runtime, Symfony DIC will automatically replace this placeholder with the actual value of the JWT_TOKEN_TTL env var, and eveything will work correctly.

So the setup is correct. The obstacle here is:

• The LexikJWTAuthenticationBundle semantic config checks at build time that the value is numeric...
• Whereas it can not be the case when using this "new" %env()% syntax, even in the end we will well and truly receive a numeric value - but at runtime only, not a build time!

The only solution I could see would be to remove that "is numeric" check in the semantic configuration, and maybe add such a check in a PHP class when it receives the TTL to counterbalance that removed check?

[chalasr]

Ok got it! My reproducer was wrongly configured, sorry about the confusion.
Happy to see that we are ready to leverage this feature once this compile time validation removed.

Instead of totally removing it, I would move it at runtime. There are two places relying on the TTL which are DefaultJWSProvider and LcobucciJWSProvider, adding the same check leading to a runtime exception in their constructor should do the job.

Would you mind to work on this?

[olivierphi]

Hey @chalasr ,

Yeah, this is what I wanted to do: move the check from container build time to app runtime.
It would have been better to keep at at build time, as it allows us to have nice error messages when warming the cache rather than potential bad surprises when the app is deployed and receives it first requests, but in that case I don't think it's possible to keep at at build time.

I can take charge of this!
This is precisely what I wanted to do, in fact, but I wanted to be sure that you would agree with that change before I start working on it :-)