Introduce utils dispatcher

[cicnavi]

I am trying to find a way to make SSP code base easier to unit test in a non-breakable way for current release.

What I have noticed is that different utility classes are often being used without the possibility to inject them, that is without the possibility to mock them.

I have gone ahead and created a "utility dispatcher" class and then injected it into several constructors where it could be used. I have provided a few examples on how it can be used in unit tests with previously "hard to mock" cases. I think with this approach we can more easily cover basically the whole SSP with unit tests (which would still be a lot of work)...

What do you think about this?

[tvdijen]

Pretty cool @cicnavi !

[cicnavi]

Alright then, will continue working on this when I "invent" some free time!

[tvdijen]

Only a matter of less sleep and more coffee ;)

[pradtke]

@cicnavi I often have the same pain with unit testing, and I'm intrigued by your approach.

Do you know how this behaves when the processing chain is running and an authproc filter performs a redirect? My impression is that all authproc filters that have not yet run will be serialized and stored in the session, so they can be resumed after. So in the current world, where all the dependencies are created with new, none of those dependencies are fields in the authproc objects and there is no issue. If the dependencies are now fields in the authproc objects then they will also get serialized, perhaps unsuccessfully. Of course the specific authproc class could implement php features to control serialization/unserialization, but I don't think the need for that is always obvious when writing an authproc filter. Anyhow, I'm curious if you've tried this approach with authproc filters (since I didn't see any in the sample PR).

[cicnavi]

@cicnavi I often have the same pain with unit testing, and I'm intrigued by your approach.

Do you know how this behaves when the processing chain is running and an authproc filter performs a redirect? My impression is that all authproc filters that have not yet run will be serialized and stored in the session, so they can be resumed after. So in the current world, where all the dependencies are created with new, none of those dependencies are fields in the authproc objects and there is no issue. If the dependencies are now fields in the authproc objects then they will also get serialized, perhaps unsuccessfully. Of course the specific authproc class could implement php features to control serialization/unserialization, but I don't think the need for that is always obvious when writing an authproc filter. Anyhow, I'm curious if you've tried this approach with authproc filters (since I didn't see any in the sample PR).

No, I didn't test with authprocs... I know resources or callbacks can't be serialized, but do you have an example on the unsuccessful serialization for properties which are objects? Below is a simple script just to get us on the same page regarding this...

#!/usr/bin/env php
<?php

class First
{
    protected string $hello = 'Hi, I\'m first';

    public function sayHi(): string
    {
        return $this->hello;
    }
}

class Second
{
    protected string $hello = 'Hi, I\'m second';
    protected ?First $first = null;

    public function __construct()
    {
        $this->first = new First();
    }

    public function sayHi(): string
    {
        return $this->hello;
    }

    public function getFirst(): ?First
    {
        return $this->first;
    }
}

$secondSerialized = serialize(new Second());

var_dump($secondSerialized);

/** @var Second $secondUnserialized */
$secondUnserialized = unserialize($secondSerialized);

var_dump($secondUnserialized->sayHi(), $secondUnserialized->getFirst()?->sayHi());

[tvdijen]

Authprocs are not serialized, just their configs in idp/sp metadata... Unless I'm horribly mistaking

[pradtke]

@cicnavi Thanks, it may have just been something specific to one of our authprocs, or me misremembering, or maybe it was us having dependencies containing secrets that we didn't want being incorporated in the $state and potentially ending up in our store. If I come across a concrete example I'll update this.

@tvdijen I believe ProcessingChain is assigning the instantiated authproc objects into the $state and $state is serialized when stored.

    $state[self::FILTERS_INDEX] = $this->filters;

During a resume, it invokes them directly form the state

        $filter = array_shift($state[self::FILTERS_INDEX]);
        try {
            if ($filter->checkPrecondition($state) === true) {
                $filter->process($state);
            }
        } catch (Error\Exception $e) {

which is why I think it is serialized/unserialized.

[tvdijen]

Hmm, you may be right. I think any object can be serialized, but private properties can become a problem. Also properties that hold items that cannot be serialized (DOMElement for example) can be problematic.

So, bottom line is, authprocs should have protected properties and if one of the properties hold unserializable items they have to implement the Serializable-interface. You can find examples for this in saml2 NameID or AttributeValue.

[cicnavi]

Cool, I wasn't aware that DOMElement can't be serialized. Do you have any example on the private properties being an issue with serialization? In this basic example it seems to work fine (the class 'Third' holds the private property '$second').

#!/usr/bin/env php
<?php

class First
{
    public string $hello = 'Hi, I\'m first';

    public function sayHi(): string
    {
        return $this->hello;
    }
}

class Second
{
    public string $hello = 'Hi, I\'m second';

    public function __construct(protected First $first)
    {
    }

    public function getFirst(): First
    {
        return $this->first;
    }
}

class Third
{
    public string $hello = 'Hi, I\'m third';

    public function __construct(private Second $second)
    {
    }

    public function getSecond(): Second
    {
        return $this->second;
    }
}

$thirdSerialized = serialize(new Third(new Second(new First())));

var_dump($thirdSerialized);

/** @var Third $thirdUnserialized */
$thirdUnserialized = unserialize($thirdSerialized);

var_dump(
    $thirdUnserialized->hello,
    $thirdUnserialized->getSecond()->hello,
    $thirdUnserialized->getSecond()->getFirst()->hello,
);

[tvdijen]

I was slightly wrong when I mentioned this last night..
For our saml2-library we came up with a solution to be able to serialize DOMElements:
https://github.com/simplesamlphp/xml-common/blob/master/src/SerializableElementTrait.php#L77-L79

However this solution won't work if the object has any private properties, because get_object_vars() wouldn't show them.. Their values would get lost during serialization/de-serialization.

[tvdijen]

@cicnavi How much work/time do you need to finish this?

[cicnavi]

I apologize, I don't know :(, can't find time to work on it...

[tvdijen]

No problem! I was just wondering if this is something we had to wait on for the 2.2 release..

[cicnavi]

I don't have a slightest idea on when to work on this. Will close for now and maybe reopen sometime in the future... :/