Remove inline signaling from Key

[donatj]

Currently Lcobucci\JWT\Signer\Key utilizes a magic sentinel 'file://' to decide whether or not to read key content from a file.

Inline signalling like this can be troublesome and more than a little dangerous at times. It can also potentially (however unlikely) lead to accidentally attempting to read from a file.

As an alternative I would like to propose perhaps a static secondary constructor "fromFile" where a user could specifically request a file's contents.

It's not a large change, but adds a lot in the way of safety. As v4 final is not yet tagged I hope this breaking change isn't asking too much.

[donatj]

The Travis error appears unrelated. The scrutinizer error about Coverage dropping I can't explain, I'm still seeing 100% coverage across the board

[lcobucci]

@donatj you're completely right regarding the interface issues and I thank you for putting effort here ❤️

I was planning on taking this bc break to the next level and convert key into an interface and allow for different implementations. I'm still brewing the idea but would you be interested in discussing this with me?

[donatj]

Oh yeah, absolutely! Honestly I think that's a great idea. I was just trying to minimize change, but if breaking them into separate classes sharing an Interface is an option, that honestly seems like the much better option.

[donatj]

@lcobucci Would you be interested in me mocking something up along those lines? I should have a fair bit of free time in the next couple days and would be willing to throw some time at it if you so desired.

[lcobucci]

@donatj that would be lovely! I'll share my thoughts here tomorrow for you to analyse and use if you agree with (I'm eager to see your ideas too).

[lcobucci]

Alright, so the rough draft I have in my mind is something like this (names aren't quite good and I might have overlooked things):

namespace Lcobucci\JWT\Signer
{
    interface Key
    {
        public function asString(): string;
    }
}

namespace Lcobucci\JWT\Signer\Key
{
    interface PemCompatible extends \Lcobucci\JWT\Signer\Key
    {
        public function path(): ?string;
        public function passphrase(): string;
    }

    final class SafeString implements \Lcobucci\JWT\Signer\Key
    {
        private string $content; // we can require php 7.4 in a separate PR (or leave it for later on)

        private function __construct(string $content)
        {
            // we can require a minimum length here to avoid having people shooting themselves      
            $this->content = $content;
        }

        public static function plainText(string $content): self
        {
            return new self($content);
        }

        public static function base64Encoded(string $content): self
        {
            return new self(base64_decode($content));
        }

        public function asString(): string
        {
            return $content;
        }
    }

    final class Certificate implements PemCompatible
    {
        public static function localFile(string $path, string $passphrase = ''): self
        {
            /* ... */
        }

        public static function content(string $content, string $passphrase = ''): self
        {
            /* ... */
        }
    }
}

What do you think @donatj?

What I like about this kind of design is that it allows us to implement JWK (#32) without breaking BC again.

[donatj]

That's slightly different than I had been thinking, but you know the problem domain far better than I do ;)

I'd probably omit the plainText that just invokes the constructor, but that's a personal style thing more than anything. Other than that I think this is a solid design.

[bradjones1]

I think a re-roll would pass tests now that PHP 8.0 support has landed.

[lcobucci]

I was playing around with the design I've suggested and it creates more confusion than what we currently have... it's kind of hard to know when to use one thing or another and some mutations are being added and I'm not quite happy about it 😬

I do think we must have an interface here, I just need to find the point to draw the line...

[lcobucci]

After thinking more about this, I realised that I was trying to solve 2 things with my suggestion:

1. Create the abstraction layer
2. Optimise memory consumption when using OpenSSL and keys/certificates in files (that's where file:// comes from)

I'll create another PR with only the abstraction layer, we can optimise performance in another moment.