BLT-481: BLT/SimpleSAMLphp Integration

[dooleymatt]

This adds phing targets that automate some of the manual steps needed to install SimpleSAMLphp on a BLT project.

[dpagini]

Hey @dooleymatt!
What is the goal of the gitignore.txt file? I haven't been able to figure that out. What are you trying to exclude from the artifact's vendor/simplesamlphp/simplesamlphp directory?

[dooleymatt]

Hey @dpagini!
The purpose of the gitignore.txt file is to replace the one that ships with the SimpleSAMLphp library. The reason that needs to be replaced is because it ignores the config and metadata directories, which prevents them from being committed to the deploy artifact. So I'm actually trying to prevent the exclusion of files from the deploy/vendor/simplesamlphp/simplesamlphp dir. Basically, it just removes these 2 lines from the gitignore file that comes with the library: https://github.com/simplesamlphp/simplesamlphp/blob/master/.gitignore#L3-L4

[dpagini]

Oh! Ok, that makes sense now! Could we just delete that file instead, maybe? Wouldn't the only real reason you need that file is if you're contributing back to that library, right?

[dooleymatt]

@dpagini true, it might not be needed at all. I will look into that.

[danepowell]

This is awesome, it's going to save a ton of project time.

@grasmash we should prioritize getting this in. It looks pretty good to me, well documented, etc...

[grasmash]

Please add this file to the mkdocs.yml navigation tree and reference this document in project-tasks.md.

[grasmash]

Rather than relying on new variable, could we instead just verify that simple saml php is being used in the project via composer info or something similar?

[dooleymatt]

@grasmash the only problem I see with that is if a project has implemented its SimpleSAML installation some other way. The simplesamlphp_auth module or the simplesamlphp library could be in composer.json.

[grasmash]

I'd also love to have some type of test here. If not a PHPUnit test, then something in BltDoctorCommand that checks for very obvious problems. E.g., if we're requiring the simple saml php library but there is no top level simplesamlphp directory, display an error. I'm sure there are a few other simple tests of that type.

[dooleymatt]

@grasmash I agree, at the very least I would like to add something to the BltDoctorCommand.

[dooleymatt]

@grasmash

• I've update the simplesamlphp.xml, deploy.xml, and setup.xml to check for the simpelsamlphp.settings.php file in the blt/settings directory, rather than checking the property in project.yml.
• I've added the readme to the mkdocs.yml
• Still need to add something to the BltDoctorCommand to do a basic check of the setup.

[dpagini]

I know I commented already, but just wanted to add a few thoughts. I do like this addition and can't wait to convert our approach to this.
Would you consider a name with just "saml", especially for the phing targets (phing/tasks/saml.xml)? If you wanted to extend to use a different library, this naming makes more sense.
Also, back to the .gitignore file... I have a few alternative ideas to attempt to make it more clear to the user what is happening.
a) I definitely think deleting the existing file would be more explanatory to a user looking at this.
b) Alternatively, could you copy in a .gitignore file that has just the lines:

!config/
!metadata/

...which is a little more self-explanatory, in that it reads do not ignore these directories.
c) Or maybe the phing target actually appends those two lines to the end of the existing .gitignore file, the way it inserts the blt.settings.php include into settings.php...? Something like:

<echo>Do not gitignore simplesamlphp library configuration files.</echo>
<exec dir="${reporoot}/vendor/simplesamlphp/simplesamlphp" command="grep !/config .gitignore || echo '!/config' >> .gitignore" logoutput="true" checkreturn="true" level="info"/>
<exec dir="${reporoot}/vendor/simplesamlphp/simplesamlphp" command="grep !/metadata .gitignore || echo '!/metadata' >> .gitignore" logoutput="true" checkreturn="true" level="info"/>

d) Last, maybe just an inline comment in the phing tasks?

[dooleymatt]

@dpagini thanks for the recommendations and please let me know if you have others.

Since this specifically adds the simplesamlphp_auth module as a dependency, which in turn adds the simplesamlphp library, I'm not sure how it could be extended to use a different library. So I am not sure that changing the names of the targets would be any more informative. At least not unless this got updated in the future to make it possible to use different libraries.

As for the gitignore file, I am happy to look into the examples you provided, however I would want to test it out first to make sure only the files we want/need are being included in the deploy artifact. It would be a pretty simple change, but I also don't think that the way it is set up now is a blocker for getting this into BLT.

Lastly, I've added inline comments to all the new simplesamlphp targets. Is this what you are referring to, or is there somewhere else you think comments need to be added?

[dpagini]

Right, for sure, nothing blocking inclusion...

For comments, I'd say either at the top of gitignore.txt or lines 68/77 of phing/tasks/simplesamlphp.xml. Just something to say why this custom .gitignore with all these exclusions is being included.

So the use case I am suggesting with phing/tasks/saml.xml vs phing/tasks/simplesamlphp.xml - if someone wanted to use simplesamlphp/saml2 (or some other saml library) instead of simplesamlphp/simplesamlphp, I could override a saml.xml file in my local repo, and then calls to phing targets saml:build:config (from setup.xml) or saml:deploy:config (from deploy.xml) can be overridden in my custom file, but use that different library.