Feature/http client by tvdijen · Pull Request #2574 · simplesamlphp/simplesamlphp

tvdijen · 2025-12-10T22:24:15Z

No description provided.

monkeyiq · 2025-12-15T05:11:29Z

Let me know when the draft is ready for a looking at. If that is now then I'm happy to kick the tyres.

tvdijen · 2025-12-15T08:09:32Z

Oh you can take a look! I still have to fix \SimpleSAML\Utils\HTTP::fetch in the same way.
We also need to take a look at the modules. I know some use guzzle as http-client now.

modules/admin/src/Controller/Config.php

monkeyiq · 2025-12-16T01:06:52Z

I am thinking that around the creation of HttpClient::create the $proxy / $proxyAuth code might need to be replicated a bit short term. Also maybe we want to Logger warn the user to move the proxy setting to the symfony config for the http-client.

So instead of calling HttpClient::create directly it makes sense to either add a createHttpClient method in \SimpleSAML\Utils\HTTP or make a HTTPClientFactory type class there. If it is a dedicated factory class or a static factory method in an existing HTTP class is less important IMHO.

Since the HTTP class already has the fetch which is client related we could just add the createHttpClient in HTTP for now and maybe move those out later if desired.

ioigoume · 2025-12-16T10:24:51Z

@tvdijen and @monkeyiq what about the redirects?

tvdijen · 2025-12-16T11:22:05Z

They are a different story. This is about SimpleSAMLphp making an outbound connection.
Redirects should be handled with a Symfony RedirectResponse. I've done that in the master-branch already

ioigoume · 2025-12-17T10:16:45Z

src/SimpleSAML/Metadata/MetaDataStorageHandlerXML.php

                Assert::isArray($config['context']);
                $context = $config['context'];
            }
+            $httpUtils = new Utils\HTTP();


@tvdijen Will the dependency injection work here?

routing/services/simplesamlphp.yml

--- services: # default configuration for services in *this* file _defaults: public: false SimpleSAML\Configuration: factory: ['SimpleSAML\Configuration', 'getInstance'] SimpleSAML\Session: factory: ['SimpleSAML\Session', 'getSessionFromRequest'] SimpleSAML\Auth\AuthenticationFactory: class: SimpleSAML\Auth\AuthenticationFactory arguments: - '@SimpleSAML\Configuration' - '@SimpleSAML\Session' SimpleSAML\Utils\HTTP: class: SimpleSAML\Utils\HTTP

and then:

protected function __construct(array $config, private ?Utils\HTTP $httpUtils = null) { $this->httpUtils ??= new Utils\HTTP();

which will also be backwards compatible.

I must say I have no idea how this is supposed to work, but of course we can try?

I like the idea of using the Autowiring stuff.

I had held off mentioning it on this PR to get the code up and running.

One possible follow up for a wider scope we might like to consider things like calls to Configuration::getInstance() to have the instance passed to the ctor by autowiring. Larger changes though.

That much wider scope has a 2.6 feel to it ;)

If we can set the example for this case, the rest can go into 2.6... I don't want to wait much longer with releasing 2.5

That sounds like a good plan. Focus on the http client updates getting in and the routing stuff can be 2.6.

monkeyiq

This looks good. The CI seems to be in a bit of a hassle.

I haven't tested this here using a proxy, that is probably a good thing before we roll this out in 2.5.

monkeyiq · 2025-12-21T01:24:02Z

config/config.php.dist


    /*
+     * @DEPRECATED: set your proxy using environment variables.
+     *


If the symfony http client is honoring the proxy and noproxy lines from
https://symfony.com/doc/current/http_client.html#http-proxies it might make sense to include a reference to that at some stage.

This might be a job for a follow up PR as we don't seem to have a config/packages/framework.yaml at the moment so adding that, explaining where that is, and also then referencing it here to mention the symfony docs for proxy as the new non-deprecated way to set things.

monkeyiq · 2025-12-21T01:25:59Z

src/SimpleSAML/Metadata/MetaDataStorageHandlerXML.php

-        } else {
-            throw new \Exception("Neither source file path/URI nor string data provided");
-        }
+        $entities = SAMLParser::parseDescriptorsString($srcXml);


This is a lot cleaner. Focus being very much on the parseDescriptor and not on if the data comes from a string or a file.

monkeyiq · 2025-12-21T01:33:38Z

There is also a reference to the curl stuff here which doesn't seem to be needed any more with this PR...

simplesamlphp/modules/admin/src/Controller/Config.php

Line 289 in 4398438

'curl_init' => [

monkeyiq · 2025-12-21T01:34:54Z

We could maybe also delete a translation

simplesamlphp/modules/admin/locales/fi/LC_MESSAGES/admin.po

Line 268 in 8212ca7

 msgid "The cURL PHP extension is missing. Cannot check for SimpleSAMLphp updates." 

monkeyiq · 2025-12-21T01:36:28Z

I thought I would dig around a bit to see what else we might like to do/add before we start considering the merge for this and the attention moved to the modules that we can refresh.

monkeyiq · 2025-12-21T02:07:39Z

You mentioned a few modules. Looking at metarefresh I see it using $httpUtils->fetch. I see that SAMLParser is also using fetch in parseFile. I am wondering if we want to hunt down the fetches right here or reimplement fetch using httpClient which would unify the client sites for us.

[ED: using HTTP::createHttpClient that is]

tvdijen · 2025-12-21T18:38:34Z

We could maybe also delete a translation

simplesamlphp/modules/admin/locales/fi/LC_MESSAGES/admin.po

Line 268 in 8212ca7

msgid "The cURL PHP extension is missing. Cannot check for SimpleSAMLphp updates."

I don't think we can. Symfony's http-client is also using curl, so it is still a required extension

monkeyiq · 2025-12-26T01:31:06Z

The CI doesn't seem to like AuthnContextComparisonTypeEnum being passed from $rac['Comparison'] to setAttribute.

When I test with that here I can force the $rac to be Constants::COMPARISON_BETTER and the call $e->setAttribute('Comparison', $rac['Comparison']); proceeds fine.

tvdijen · 2025-12-26T11:41:35Z

I've fixed that in the release-branch, but I still have to rebase this one

tvdijen force-pushed the feature/http-client branch from 381f63b to 941c8a7 Compare December 10, 2025 22:25

tvdijen mentioned this pull request Dec 10, 2025

Replace different http-client approaches with a generic PSR-18 client #2570

Open

tvdijen force-pushed the feature/http-client branch from cb14698 to ec470ce Compare December 10, 2025 22:55

tvdijen force-pushed the simplesamlphp-2.5 branch from 98f855c to e9b1651 Compare December 10, 2025 22:57

tvdijen force-pushed the simplesamlphp-2.5 branch 2 times, most recently from 4c158e2 to 8e1da16 Compare December 15, 2025 09:21

monkeyiq requested changes Dec 16, 2025

View reviewed changes

modules/admin/src/Controller/Config.php Outdated Show resolved Hide resolved

tvdijen force-pushed the feature/http-client branch 2 times, most recently from 8bde086 to 3c746a7 Compare December 16, 2025 12:23

tvdijen added 6 commits December 16, 2025 13:27

Fix lock-file

276a609

Fix lock-file

8f09cdd

Migrate version-check to http-client

b08fd46

Deprecate proxy-settings

f219a09

Remove duplicate parenthesis

e00cc5e

Move instantiation of the HttpClient to the HTTP-utility

2355f18

tvdijen force-pushed the feature/http-client branch from 3c746a7 to 3359182 Compare December 16, 2025 12:27

tvdijen added 4 commits December 16, 2025 16:01

Add deprecation notice for HTTP::fetch

bf3a39a

Migrate MDQ to use httpClient

50fb657

Improve error handling

8004457

Migrate XML storage handler to use a httpClient

91766d0

tvdijen force-pushed the feature/http-client branch from e1dd008 to 91766d0 Compare December 16, 2025 15:45

tvdijen added 2 commits December 16, 2025 16:45

Bump test-framework

67f3214

Fix deprecation-attributes

83d976c

ioigoume reviewed Dec 17, 2025

View reviewed changes

Add a service for the HTTP-utility

1206c4a

tvdijen force-pushed the simplesamlphp-2.5 branch from 4874a87 to 4398438 Compare December 18, 2025 20:44

monkeyiq reviewed Dec 21, 2025

View reviewed changes

tvdijen closed this Dec 21, 2025

tvdijen reopened this Dec 21, 2025

monkeyiq added 2 commits January 8, 2026 12:59

reference to docs for options

4ec22a8

a first attempt to move these to using symfony httpclient as well

427cc3e

Conversation

tvdijen commented Dec 10, 2025

Uh oh!

monkeyiq commented Dec 15, 2025

Uh oh!

tvdijen commented Dec 15, 2025

Uh oh!

Uh oh!

monkeyiq commented Dec 16, 2025

Uh oh!

ioigoume commented Dec 16, 2025

Uh oh!

tvdijen commented Dec 16, 2025

Uh oh!

ioigoume Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvdijen Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

monkeyiq Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

monkeyiq Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

tvdijen Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

monkeyiq Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

monkeyiq left a comment

Choose a reason for hiding this comment

Uh oh!

monkeyiq Dec 21, 2025

Choose a reason for hiding this comment

Uh oh!

monkeyiq Dec 21, 2025

Choose a reason for hiding this comment

Uh oh!

monkeyiq commented Dec 21, 2025

Uh oh!

monkeyiq commented Dec 21, 2025

Uh oh!

monkeyiq commented Dec 21, 2025

Uh oh!

monkeyiq commented Dec 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tvdijen commented Dec 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

monkeyiq commented Dec 26, 2025

Uh oh!

tvdijen commented Dec 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ioigoume Dec 17, 2025 •

edited

Loading

monkeyiq commented Dec 21, 2025 •

edited

Loading

tvdijen commented Dec 21, 2025 •

edited

Loading