Implement prs-7 request/response handling

[Hikariii]

At the moment this library does a lot of request parsing and response handling on it's own.
This issue proposes moving to psr-7 for request/response handling.

Bypass and/or resolve current and possible future issues:

• Assumptions are made about the incoming request and superglobals are being interpreted sometimes leading to misbehaving code.
  • https://github.com/onelogin/php-saml/blob/master/lib/Saml2/Utils.php#L363-L382
  • https://github.com/onelogin/php-saml/blob/master/lib/Saml2/Utils.php#L402-L435
  • Add security config to disable destination check #149 (comment)
• The toolkit (in some cases) sets headers and exits (in case of redirecting). It is possible to circumvent, but it is considered bad practise to even have the option of exiting php within a library.
  • https://github.com/onelogin/php-saml/blob/master/lib/Saml2/Utils.php#L270-L279

There is a standard, please use it:

Up until last year here was a lot of different request/response handling going on within different frameworks. Since psr-7 a lot of frameworks have moved to prs-7 for their response/request handling.
Just check https://mwop.net/blog/2015-01-26-psr-7-by-example.html and how this is a great standard for handling this.

Increase interoperability and collaboration:

Using psr-7 internally would greatly improve the integration in different frameworks and would make it a lot easier to write middleware around php-saml. Requests and responses could be interpreted and returned in a standardized psr-7 way, clear for everyone who uses it.

Provide backwards compatibility layer for old-style usage:

To provide a form of backwards compatibility the library could provide it's own utility to create and resolve psr-7 requests and responses for usage outside of a framework. I actually forsee that most of the implementations will actually use this library within a framework and do not need this layer, but it's still nice to have and be able to use this library in a stand-alone way.

Requires a lot of refactoring

This is probably something for a 3.* version, because it will change the outer api.
Internally it will probably be quite some work to upgrade this, but it will definitely improve adoption and ease of use for this library within the context of bigger applications.

[pitbulk]

prs-7 seems interesting and something to take in mind. Thanks or sharing this.
Maybe if people collaborate with can release that in a 3.* version.

[chris001]

@Hikariii @pitbulk

This is probably something for a 3.* version, because it will change the outer api.

It should probably be done by adding another interface which consumes and produces psr7 type format parameters, while keeping the old pre-psr7 interface which uses urls and parameters in arrays and strings. Then internally the new code can handle the data objects however is best. Probably best to add a class which does the conversion for you, and can output its contents either in psr7 format, or scheme/uri/url/parameters/cookies/etc strings format. Probably this class already exists.

[pitbulk]

I think a 3.X version makes sense. Focused on PHP7 and PRE-PSR7 and witthout the mcrypt requirement

[pitbulk]

The 3.X is now there, but we need to keep improving it in order to do an official release.
@Hikariii @chris001 do you have any progress related to port php-saml to use PSR-7?
We may also adopt PSR-4

[chris001]

@pitbulk
To accomplish this I suggest adding:

• composer and psr-4 autoloading and obviously namespace all the php source files into its own namespace to prevent collisions in the global namespace,
• guzzlehttp psr-7 it's the most popular library to implement psr-7 https://github.com/guzzle/psr7 it's easy to use and easy to install at first run with a composer command.

git clone https://github.com/onelogin/php-saml
cd php-saml
composer require guzzlehttp/psr7 guzzlehttp/guzzle

How to use guzzlehttp psr7 it's quite simple: http://docs.guzzlephp.org/en/stable/psr7.html
Here's an OK step by step procedure for converting to PSR-4 autoloader and namespaces:
https://www.sourcetoad.com/web-training/how-we-migrated-a-legacy-code-library-into-a-modern-composer-package/

[Hikariii]

@chris001 and @pitbulk: I started an experimental rewrite a few weeks ago. Hope to finish a great deal of it this week.
For PSR-7 I use Zend Diactoros https://github.com/zendframework/zend-diactoros.
Guzzle is a http-client app and the PSR-7 Requests/Responses for SAML are for a php server-side app.

[Hikariii]

Also, I'm working with php-saml in a Symfony app and would like to use the Symfony PSR-7 bridge with php-saml to inject requests and return responses.
The PSR-7 bridge already suggests working with Zend Diactoros: https://symfony.com/doc/current/request/psr7.html

[pitbulk]

Hi @Hikariii, any progress on this?

[pitbulk]

@Hikariii I made some progress on the 3.0.0 branch. I want to know if you have time to work on it, or if I may continue your work of that branch: https://github.com/Hikariii/php-saml/commits/feature-rewrite

[stof]

What is the status of this work ? I would be interested in this, as it would integrate much more cleanly in my Symfony project.

[pitbulk]

I have not worked on that, the only work done is the one that can be found at:
https://github.com/Hikariii/php-saml/commits/feature-rewrite

Don't know how much PSR-7 conversion will affect current 3.0.0 branch. will the implementation be compatible with current implementation?
3.0.0 is not yet officially released, so I think it's ok to include PSR-7 on the official release, and create a tag 3.0.0-no-psr7 for the pre PSR-7 implementation, like I did for the 3.0.0 version with no namespaces: 3.0.0-namespaceless

You have contributed a lot today on 3.0.0 branch, do you think you gonna have time to work on the PSR-7 stuff?

[slaughter550]

@stof @pitbulk if there is some grunt work that someone is dreading doing, I can help with some of it to move this forward. I don't have enough mental bandwidth to run with the conversion but I have some spare time between CI builds that I could put towards this project.

[pitbulk]

Any help is welcome, I'm currently out of time since I had to jump in other projects.

[slaughter550]

@pitbulk Did you like the idea of using zend-diactoros or would you be open to using guzzle/psr7? I have substantially more experience with guzzle but happy to learn more about zend-diactoros if thats the direction you want to go.

[pitbulk]

Based on @Hikariii I think makes more sense to use zend-diactoros

[slaughter550]

Sounds good, I'll review what @Hikariii did and keep pushing forward

[stof]

Reviewing what @Hikariii has done is almost impossible, as he deleted the whole codebase and added the new code in a different folder (making any git diffing unusable)

[Hikariii]

@stof I agree that I did do far too many changes to the codebase to come to a PR that is diffable. The work I did ended up to be a restructure/rewrite combined with integrating new features(too much going on).
I think the way forward must include a lot more smaller steps. The hard part about that is that taking a request/response object as input/output is inherently different from the way the codebase works now.

The codebase heavily uses php globals and gets it input from $_GET and $_POST. Even more, the code also writes its own headers and even exits (can be bypassed).
I explored a way to really use a request/response object as a base, but concluded that it came down to a lot of rewriting.

I do think it is wise to explore a way of restructuring the code in a way that it can be reviewed and refactored nicely and in smaller bits.

[jorijn]

@Hikariii is this still on your radar somewhere?

[Hikariii]

@jorijn Nope not really. Refactoring this in a structured way requires a lot of planning/testing/rewriting and probably some help. It is not a priority (anymore).

[joelharkes]

Doesn't seem to much work to simply make the $_GET variable an optional input parameter?

https://github.com/search?q=repo%3ASAML-Toolkits%2Fphp-saml%20%24_GET&type=code

Its only in Auth and LogoutRequest?