-
Notifications
You must be signed in to change notification settings - Fork 135
Library
Back to Background
Creating the library from the current SSP code.
BB:
Basically the 2 library folders in lib/ have moved to their own repos and using Composer these can be loaded into vendor/. SSP can then use the Composer autoloader and add to it only the loading of modules.
- Ran the php-cs-fixer to make it PSR-0, PSR-1 and PSR-2 compliant.
- Basic integration of Travis CI [PM: Updated link to current lib]
- Basic integration of various QA tools: PHP Mess Detector, PHP CodeSniffer, Security Advisory Checker, PHP Copy Paste Detection, linting
- Example setup for Unit Testing with 2 simple tests
- Improve phpDoc (discovering some minor issues in the progress)`
What we'd like to do:
- Improve code coverage (eventually maybe even reaching 100%?).
- Add documentation to the project.
- Move more SAML2 functionality from module/saml to the saml2 library.
- Make the saml2 library work without the libsimplesaml library.
This if off course only an example, to show you our thoughts about separating the SAML2 library. We've explicitly kept BWC so as to not veer too far off the main SSP codebase."
OM:
Basically, what I'd like in a library is:
- Relatively low-level classes to parse and generate the XML. Something like what you can find in lib/SAML2/XML/.
- Helper-classes for the SAML 2.0 bindings.
- Helper-classes for implementing a SAML 2.0 SP and IdP.
Note that the helper-classes should only be helper-classes. No session management, no backends for storing metadata, or anything like that. Instead, the final application should use the helper-classes (possibly through subclassing), and supply their own functionality for those parts. The goal here would be to create something that can be tightly integrated into existing applications and web frameworks, which have their own session management, configuration system and user database backend.
However, I think creating such a library requires more than just taking the current library, and cleaning it up a bit. I think such a library needs an interface that is stable, and I am not happy about the current state of the SAML2-library in simpleSAMLphp.
I also don't think moving the code from the saml-module to the library is doable, since it is all rather tied to the way simpleSAMLphp manages metadata, sessions and configuration.
BB:
Yes, that's unfortunate (although surprisingly not that much of a problem seeing as the dependencies are relatively minor and can easily be 'mocked'). I would very much like to remove all static calls from the codebase and replace them by calls to injected dependencies.
Basically what we're looking for is:
- XML marshalling / unmarshalling.
- Object model with static guarantees (so I can do
$response->getAssertion(0)->getIssuer()
). - XML DSig / Enc helpers (I never want to have to touch signature verification again :-) )
- Ideally: a large amount of configuration freedom (preferably using Dependency Injection or a similar technique).
OM:
I think all the changes you have made and are proposing are a step in the right direction. However, if we are going to change everything, I would go even farther.
We have been thinking a bit about a future simpleSAMLphp 2.0, where we get the chance to break backwards compatibility with the current system, and thus can make bigger changes to the entire code. Splitting out the SAML 2.0 functionality into a library has been high on our wishlist for that.
BB:
I hope I'm not being too forward if I caution against the Second-system effect (if you haven't read it yet Joel Spolsky gives an amusing account in Things You Should Never Do).
OM:
Yes, I have read it. Note that I am not thinking about throwing away the current code, and starting from scratch, nor of adding a lot of new features. I am thinking about significant refactoring/reworking of the code. The goal would be to get rid of a lot of cruft, and have simpler and clearer code afterwards. At the same time, some unfortunate inconsistencies and design choices can be fixed.
Basically, the code has grown a lot from its beginning, but because we couldn't predict the future we are left with some old design errors and code that should be fixed. However, that is an entirely different task from separating out the SAML 2 library.
BB:
SURFnet was in a similar situation before with Corto where we made significant changes, in agreement with upstream, that eventually were not accepted in trunk, leaving us with a fork.
That's why I took special care to make everything in the SimpleSAMLphp example as BWC as possible. We are hesitant to make too drastic a change and risk being stuck with an unused fork, giving us all the negatives (support for functionality we don't use) and none of the benefits of shared collaboration. We would much rather work first on a 1.12 with refactorings to initiate the cooperation and prepare for a 2.0. A minimal change with mutual interest and mutual support. Too much change at once would also lead to new bugs which would be counterproductive to our goal of switching to a 'proven' library.
OM:
I am only worried about locking the SAML 2 library to an API that has severe limitations / design defects. Now the SAML 2 library isn't a separate part of simpleSAMLphp, and we haven't made any promises wrt. interface stability. However, I think a separate library automatically comes with an expectation of API stability, which makes it harder to change the API later.
BB:
That's what versioning is for. We can clearly label the first version as a '0.1.0', a start.
OM:
Yes, it helps. Of course, people will still need to rewrite their code or be stuck on an old / abandoned version.
BB:
After our discussion I understand this argument better. Basically you're warning us that changes are imminent and that we (and anyone then using the library, though I don't think this will be a significant number of users for a while) would be forced to make an investment again.
We (and I'm looking at the SURFnet guys here) accept this risk. We should document it in the README and make sure that all users are aware of this. I think Pieter said it best when he said that you guys are still governing this. We will help you set it up and then simply be the first users. We'll help you flush out bugs, improve quality, add tests and minor features. But in the end we want to use the SSP SAML2 library. If you release SSP 2.0 with SAML2 1.0.0 OpenConext will upgrade.