Support aggregation of attributes from multiple databases #20

nathanjrobertson · 2025-10-22T00:53:58Z

sqlauth:SQL has support for multiple queries, however currently they must all run against the one SQL database. In larger environments, it would be useful to be able to gather attributes from several separate databases.

So that's how this one started out. In exploring that requirement, I quickly found that separating the database configuration from the query configuration was required, and my hacks to the existing ("Version 1") configuration format was getting messy.

So, this PR proposes a new configuration format for authsources.php. The new ("Version 2") format separates the three key entities - databases, authentication queries and authorization queries, and as a result allows multiple of each type:

$config = [
    [...]
    'example-sql' => [
        'sqlauth:SQL2',

        'databases' => [
            'idp' => [
                'dsn' => 'pgsql:host=postgresql;port=5432;dbname=simplesaml',
                'username' => 'simplesaml',
                'password' => 'secretpassword',
            ],
        ]

        'auth_queries' => [
            'auth_username' => [
                'database' => 'idp',
                'query' => "select uid, givenName as \"givenName\", email from users where uid=:username and password=encode(sha512(concat((select salt from users where uid=1), :password)::bytea), 'base64')"
            ],
        ],

        'attr_queries' => [
            [
                'database' => 'idp',
                'query' => "select groupName from usergroups where uid=:username",
            ],
        ],
    ],
    [...]
];

Before anyone gets too upset, there is a full set of backward compatible classes (sqlauth:SQL1Compat, sqlauth:PasswordVerify1Compat). In addition, the existing sqlauth:SQL and sqlauth:PasswordVerify are untouched (for the minute), but the intent is that after the new code and "compat" interfaces prove stable, SQL.php becomes class SQL extends SQL1Compat {} (likewise for PasswordVerify.php), meaning people are transparently moved across to the new code without any configuration changes. So, it maintains full backward compatibility from a configuration interface point of view, but without having to drag the old configuration format baggage.

The main benefits of the new "Version 2" configuration format are:

Separation of databases, auth_queries and attr_queries makes it much clearer which entity type a given configuration parameter applies to. The "Version 1" flat hierarchy meant that as new configuration attributes were being added it was becoming increasingly unclear what concept they were applying to.
Supports the common key (:username) not being the shared common key between an Authentication query and an Attribute query. There was an issue before if somebody logged in with only a candidate key (eg. a unique email address), but the primary key shared between tables was something else (eg. a numeric customer ID), there wasn't a way to say "grab this ID in the authentication query and provide it for use in attribute queries. Now there is (extract_userid_from).
Because previously we could only have one authentication query, it was not possible to avoid an SQL error when trying to support multiple formats of username. For example, if a user can login with an integer student ID or an email address, the query select [...] from [...] where (integer_id=:username or email_address=:username) and password=[...] will cause an SQL error when trying to login with an email address (as integer_id=:username will fail with an int vs string comparison error). Multiple authentication queries with the username_regex set on each (one query for the integer version, one for the email version) gets around this.
Can gather attributes from more than one database. The new only_for_auth parameter makes it possible to only run a given attribute query if the user authenticated using one of the authentication queries referenced in an "allow list".
It is 100% feature complete compared to "Version 1". There are no missing features (including password_verify() support).

Also note:

README.md is fully updated and provides detailed examples and configuration parameter dictionary.
Lots of unit tests, fully testing both the "compat" configuration interfaces and the new "Version 2" configuration interfaces. I've updated the existing tests to allow them to be run against either the old code or the new "compat" versions. So, any unit tests written against "Version 1" will automatically be tested against "compat" versions.

I don't have a production use case for the password_verify() support, so the unit tests are the extent of my Version 2 and compat testing with that functionality. It's here such that Version 2 has all the features Version 1 did. Version 2 implements it in the main module, not as a separate PasswordVerify subclass (with much code duplication) as was done in Version 1.

Internally we're testing all of this at the moment, starting with the "compat" interfaces, then we'll switch to "Version 2" configuration, before starting to use all the multi-database support. If the project maintainers could take a look at this one and give feedback on what changes you'd like for this to be merged then that'd be great.

…_docs Add documentation for UNIX socket connections

…ation queries, attribute queries selectable based on which authentication query was successful

…es on PHP 8.1 and 8.2

codecov · 2025-10-22T03:12:24Z

Codecov Report

❌ Patch coverage is 63.96867% with 138 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.03%. Comparing base (3866f84) to head (2d7157b).
⚠️ Report is 37 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master      #20      +/-   ##
============================================
- Coverage     71.42%   66.03%   -5.40%     
- Complexity       44      148     +104     
============================================
  Files             2        5       +3     
  Lines           147      530     +383     
============================================
+ Hits            105      350     +245     
- Misses           42      180     +138

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tvdijen · 2025-10-22T10:01:08Z

@nathanjrobertson Could you please re-generate the phpstan-baseline-dev.neon file?

tvdijen

Looks really good Nathan! Let's see if Ben has any remarks.

…n what we're doing

nathanjrobertson · 2025-10-23T00:35:50Z

@nathanjrobertson Could you please re-generate the phpstan-baseline-dev.neon file?

@tvdijen Done. I ended up creating a new Interface to make the inheritance in the test hierarchy more obvious to phpstan, which brought the number of exceptions needed in the phpstan-baseline-dev.neon back down to two. All tests now pass again.

tvdijen · 2025-10-23T07:38:03Z

Perfect! Just two more things..

I've noticed all the require_once() in bootstrap.php which shouldn't be necessary. We can remove all of them if we add the proper namespace under require-dev in composer.json
There are three issues reported by scrutinizer that should be very easy to fix.

…ev in composer.json

nathanjrobertson · 2025-10-23T22:36:14Z

Perfect! Just two more things..

1. I've noticed all the `require_once()` in `bootstrap.php` which shouldn't be necessary. We can remove all of them if we add the proper namespace under `require-dev` in `composer.json`

2. There are three issues reported by scrutinizer that should be very easy to fix.

Thanks Tim. Both fixed now.

I think you meant autoload-dev, not require-dev above, yes?

monkeyiq · 2025-10-24T03:54:25Z

I will give this a more detailed viewing starting in the next days.

A bit of my initial reading is around the PasswordVerify which you mention is secondary to your use cases.

Initial thoughts are that we should move password_verify_hash_column handling from login into a member function. The login is already a bit large and moving that block into a member function should help a bit.

The intent of checking each returned $data as $row in the current PasswordVerify.php is to allow 1+ tuples to be returned from the database and make sure that the password hash is valid for all the tuples returned. If we are looking to allow a set of $authQueries and the first success is final success for auth I think for password verify it makes sense to retain the same logic that for any single query if there are 1+ tuples returned then all of those results must have the same value for passwordhashcolumn and that value must also password_verify with the supplied password.

I would like to simplify the logic of the new verifyHashColumnForQueryDB method from what is there. It seems that if the password hash does not verify against the first tuple in the result an error would be logged when password_verify failed, the continue would run the same loop as if the error didn't happen. In the simple case if there is only one tuple an error would be logged but the user would be logged in?

I am always a bit suspicious of code with a if(...) { continue; } block right at the bottom of a for/while loop.

tvdijen · 2025-10-24T08:51:36Z

I think you meant autoload-dev, not require-dev above, yes?

Yes, my bad! Thanks for all your hard work!

nathanjrobertson · 2025-10-27T00:26:37Z

A bit of my initial reading is around the PasswordVerify which you mention is secondary to your use cases.

Although I don't personally use it, I respect it needs to be there and be 100% backward compatible. I don't see it as a second class citizen - more just a feature I don't personally have a need for, but that somebody else definitely does.

Initial thoughts are that we should move password_verify_hash_column handling from login into a member function. The login is already a bit large and moving that block into a member function should help a bit.

That's fair enough. I can do that.

The intent of checking each returned $data as $row in the current PasswordVerify.php is to allow 1+ tuples to be returned from the database and make sure that the password hash is valid for all the tuples returned. If we are looking to allow a set of $authQueries and the first success is final success for auth I think for password verify it makes sense to retain the same logic that for any single query if there are 1+ tuples returned then all of those results must have the same value for passwordhashcolumn and that value must also password_verify with the supplied password.

Indeed, the intent is that the first success is the final success, and any later authqueries are never evaluated.

Hmm. My intent was to retain the existing functionality as you describe - the inner foreach loop is "foreach row returned for this query", and this elseif checks the $passwordHash is the same in all the rows. If it gets to the end of evaluating for every value in that foreach loop then all the $passwordHash values must be the same and not null.

If you can see some way I'm not covering that case then that's a bug. 100% backward compatibility with new functionality added is explicitly the goal.

I would like to simplify the logic of the new verifyHashColumnForQueryDB method from what is there. It seems that if the password hash does not verify against the first tuple in the result an error would be logged when password_verify failed, the continue would run the same loop as if the error didn't happen. In the simple case if there is only one tuple an error would be logged but the user would be logged in?

No. The only continue in the current code is for the outer foreach loop, which means "continue to the next candidate authquery (if any)", meaning this current authquery is deemed to fail. Without that continue, it'll make it to this break, which indicates we've found a successful authquery - essentially emulating what would have happened in the non password_verify() case if the if(count($data) > 0) had evaluated to false.

The code is conservative and throws an error in the case where all the tuple rows don't match in the password_verify() case rather than try the next authquery (here, here and here). The thinking is that for one of those three conditions to trigger, the user exists in the database this password_verify() authquery uses (hence, the query returned rows), but a data error occured in retrieving their hash, and we probably don't want to pass through to the next authquery in that case. The other option is to let it try the next authquery (ie. continue to the next iteration of the outer foreach loop (ie. next authquery), just like if any regular non-password_verify() authquery failed).

Let me know if you've got a preference on this one - there's no backward compatibility risk, as Version 1 never supported multiple authqueries. My feeling is that throwing the error is better, as there's a decent chance this authquery was supposed to be the winning one (given the user exists in the database) if it weren't for a data issue.

I am always a bit suspicious of code with a if(...) { continue; } block right at the bottom of a for/while loop.

The continue goes to the outermost foreach loop is "foreach authquery".

It's essentially the same as the existing PasswordVerify::login(), where after the end of the "foreach returned tuple, check all passwordhashcolumn values are the same" section. Because the old code only supports a single auth query, it throws the WRONGUSERPASS error instead of continuing on to try the next candidate authquery, like the proposed new code does.

…nction

nathanjrobertson · 2025-10-27T01:03:17Z

Initial thoughts are that we should move password_verify_hash_column handling from login into a member function. The login is already a bit large and moving that block into a member function should help a bit.

That one is fixed. Yes, it is a fair bit tidier and easier to read. Thank you.

…layer

…le-sqlauth into sql2

nathanjrobertson · 2025-11-03T06:03:51Z

Just the one bug picked up in our first round of integration testing - extra config like core:loginpage_links were being dropped when translating the configuration from V1 to V2 in the Compat interfaces. I've just pushed that fix into the branch for this PR.

monkeyiq · 2025-11-02T02:10:17Z

src/Auth/Source/SQL2.php

+                    $hashColumn,
+                ));
+                throw new Error\Error('WRONGUSERPASS');
+            } elseif (strlen($row[$hashColumn]) === 0) {


I think this strlen test is better as just a raw if near the top of this foreach (above line 448 for example). The strlen($row[$hashColumn]) must always be > 0 regardless of anything else that is happening.

I can analyse that if $passwordHash === null and strlen == 0 and $passwordHash != $row[$hashColumn] will not be true when $passwordHash == null and strlen == 0, so the final elseif will run. But it is simpler logic to just lift the strlen($row[$hashColumn]) > 0 test out of this if/elseif/elsif block. If it is a single if() for every result inspected in the result set it is very clear it always runs and negates bad data without much thought needed.

Yep, fair enough. I think I've captured the above in a495d6b.

monkeyiq · 2025-11-02T23:14:58Z

docs/sql.md


-Options
-------
+- Most commonly, as a part of the SQL query itself (ie. using SQL functions to hash a parameterized password and compare that to a value stored in the database).


These docs are great stuff!

Thanks. What gets documented (well) gets used.

monkeyiq · 2025-11-07T00:32:41Z

src/Auth/Source/SQL2.php

+            }
+        }
+
+        // At the end, disconnect from all databases


if $db is still set from above then at least one database will be reported as disconnected but will still be open?

Yep. Fixed in 6b4e981.

I've been thinking through this one in the past few days. I think after this PR is merged I'll write and submit some improvements to the cleanup process. In the case where there are multiple databases on the same physical server, it means we are holding a number of connections open by the end, and we could easily hit a connection limit. I'm expecting to have this problem myself - we might have 5-10 attribute queries against 5-10 different databases on the one physical database host, and if there are concurrent connection attempts that could cause us to run out of connections.

My thinking is to be a bit more smart and aggressive on reading ahead as to whether a connection needs to be retained, and if not, closing it earlier than the very end.

But for the minute, for this PR, I'll leave it as is. But know I'm thinking of making the disconnect cleanup process more aggressive in the future.

monkeyiq · 2025-11-07T00:35:06Z

src/Auth/Source/SQL2.php

+            }
+        } else {
+            throw new Exception('Missing required attribute \'databases\' for authentication source ' . $this->authId);
+        }


I think it is simpler for this to check if array_key_exists('databases', $config) is not true and throw and then the code can just continue from there. Having this in an else here means you have to check back to what that else is for and work it out.

If the key is not set then an exception. The code can just flow on from there without any more if/else logic.

Fixed in 630316d

monkeyiq · 2025-11-07T00:35:34Z

src/Auth/Source/SQL2.php

+            throw new Exception(
+                'Missing required attribute \'auth_queries\' for authentication source ' .
+                $this->authId,
+            );


Similar to the if/else above on line 112.

Fixed in 618e0d3

monkeyiq · 2025-11-07T00:42:32Z

I noticed that $winning_auth_query is not camelCase though many other things are. This doesn't really worry me too much, and I see that username_regex in the previous SQL.php was also using this convention so I have just left this here as a comment rather than inline.

nathanjrobertson · 2025-11-10T04:18:05Z

Thanks for the feedback @monkeyiq. I'll get to addressing it all later this week or early next week.

nathanjrobertson · 2025-11-11T07:04:31Z

I noticed that $winning_auth_query is not camelCase though many other things are. This doesn't really worry me too much, and I see that username_regex in the previous SQL.php was also using this convention so I have just left this here as a comment rather than inline.

Now that you've pointed that out it annoyed me enough to fix the variable name. Fixed in 9b83f78.

username_regex was also my fault. I think underscores are ok for configuration keys in authsources.php, but variables in the code should be consistent and camelCase.

Thanks for pointing that out.

…0 to ~1.6.0

nathanjrobertson · 2025-11-24T22:31:09Z

@monkeyiq Quick bump - this one is ready for re-review. Thanks.

monkeyiq · 2025-11-24T23:38:36Z

I am planning on taking a further look at the Use PSR-14 events PR and then returning to this. The interaction of the PSR-14 and SSP 2.5 release date sort of pushed that PR to the top. I haven't forgotten about this PR and will return to it likely on the weekend or soon after. Sorry about the delay.

tvdijen · 2025-11-25T07:12:21Z

Oh whoops, that wasn't supposed to happen :') Please keep this on your radar @monkeyiq

nathanjrobertson and others added 14 commits October 13, 2025 09:16

Add documentation for UNIX socket connections

b8bb0e6

Spelling fix

d4efc2e

Merge pull request simplesamlphp#19 from nathanjrobertson/unix_socket…

f50130b

…_docs Add documentation for UNIX socket connections

Implement sqlauth v2 - support multiple databases, multiple authentic…

2286d34

…ation queries, attribute queries selectable based on which authentication query was successful

Support password_verify() in version 2. Provide v1 compat interfaces.

facb328

Fix phpcs errors and warnings

9e62356

phpstan fixes

de4021b

More phpstan fixes

c0a0e77

Provide documentation for the Version 2 configuration format

51c60e7

Fix linting for README

92221a0

Fix linting

bac816c

Spelling fixes

80574d5

Run phpcs from vendor, not GitHub installed version

693a255

Fix deprecation warnings in phpunit, strangely causing phpunit failur…

833cd52

…es on PHP 8.1 and 8.2

composer install with "--prefer-dist" fails. Remove that flag.

86da39e

tvdijen force-pushed the master branch 5 times, most recently from f6a7250 to 8c21582 Compare October 22, 2025 09:37

Merge branch 'master' into sql2

26aabd0

tvdijen requested a review from monkeyiq October 22, 2025 09:48

tvdijen reviewed Oct 22, 2025

View reviewed changes

nathanjrobertson added 2 commits October 23, 2025 11:26

Have test classes use concrete interface to make it clearer to phpsta…

64142b9

…n what we're doing

Add wrapper interface to bootstrap.php

8761cb7

Remove require_all() lines in bootstrap.php. Move to using autoload-d…

6a2919e

…ev in composer.json

Fix scrutinizer issues

1230b28

monkeyiq self-assigned this Oct 24, 2025

Move password_verify_hash_column handling from login into a member fu…

4b38aa5

…nction

nathanjrobertson and others added 3 commits October 28, 2025 09:35

Merge branch 'simplesamlphp:master' into sql2

e510ce7

Fix core:loginpage_links (and other extra parameters) in SQL1 compat …

0ffef41

…layer

Merge branch 'sql2' of github.com:nathanjrobertson/simplesamlphp-modu…

d53f98a

…le-sqlauth into sql2

monkeyiq requested changes Nov 7, 2025

View reviewed changes

nathanjrobertson added 7 commits November 11, 2025 09:11

Add test case for database ID being wrong in configuration

3f511dd

Fix phpcs error

2333422

Improve readability of authenticatePasswordVerifyHash()

a495d6b

Drop reference to most recently used database in the cleanup phase

6b4e981

Make error handling for missing databases key more obvious

630316d

Make error handling for missing auth_queries key more obvious

618e0d3

Fix $winning_auth_query not being camelCase

9b83f78

nathanjrobertson requested a review from monkeyiq November 11, 2025 07:04

Upgrade simplesamlphp/composer-module-installer dependency from ~1.5.…

2d7157b

…0 to ~1.6.0

tvdijen merged commit a159dbd into simplesamlphp:master Nov 25, 2025
24 checks passed

Support aggregation of attributes from multiple databases #20

Support aggregation of attributes from multiple databases #20

Uh oh!

Conversation

nathanjrobertson commented Oct 22, 2025

Uh oh!

codecov bot commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tvdijen commented Oct 22, 2025

Uh oh!

tvdijen left a comment

Choose a reason for hiding this comment

Uh oh!

nathanjrobertson commented Oct 23, 2025

Uh oh!

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

Uh oh!

Uh oh!

nathanjrobertson commented Oct 23, 2025

Uh oh!

monkeyiq commented Oct 24, 2025

Uh oh!

tvdijen commented Oct 24, 2025

Uh oh!

nathanjrobertson commented Oct 27, 2025

Uh oh!

nathanjrobertson commented Oct 27, 2025

Uh oh!

nathanjrobertson commented Nov 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monkeyiq commented Nov 7, 2025

Uh oh!

nathanjrobertson commented Nov 10, 2025

Uh oh!

nathanjrobertson commented Nov 11, 2025

Uh oh!

nathanjrobertson commented Nov 24, 2025

Uh oh!

monkeyiq commented Nov 24, 2025

Uh oh!

Uh oh!

tvdijen commented Nov 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Oct 22, 2025 •

edited

Loading

tvdijen commented Oct 23, 2025 •

edited

Loading