Remove code/features that aren't used

[kreynen]

The D7 project evolved a lot since Robert Douglas started it 11+ years ago. I'd like to remove some of the features added in the last decade that are no longer of interest to anyone using this project in Backdrop.

• - I'm not sure the Rules integration in the module works with https://backdropcms.org/project/rules. Even if it does work, I can't imagine a use case for this that couldn't be handled by rules_user_login or rules_user_logout. @irinaz?
• - There is a check in https://github.com/backdrop-contrib/simplesamlphp_auth/blob/1.x-1.x/simplesamlphp_auth.admin.inc#L17 for the version of the simpleSAMLphp library. It is > 1.5.x. The currently supported release of simpleSAMLphp is 1.19.6. There is also an RC2 for the 2.0 release. I have no idea if this check still work. In our use case, we are adding the simpleSAMLphp library with Composer. Our security teams are not going to log into a Backdrop instance to see an in app status report message. They want to be able to scan the project's composer.json for this.
• hook_simplesamlphp_auth_logout_gotourl_alter in https://github.com/backdrop-contrib/simplesamlphp_auth/blob/1.x-1.x/simplesamlphp_auth.api.php#L17. Another case of not knowing if this works... and not being able to come up with a use case for this that couldn't be handled in the IdP logout configuration.

I'm sure there is more questionable code in this, but this is probably more of a 1.1.x change. Just wanted to get the conversation started.

[argiepiano]

Regarding point 1 (rules integration). This module defines two rules events, and makes the appropriate invocations to them in specific points of the code (within simplesamlphp_auth_user_insert(), and simplesamlphp_auth_init()).

So, from the point of view of functionality, this looks fine, and will work fine if you create a Rule that reacts to those events.

[kreynen]

That still doesn't explain the "that couldn't be handled by rules_user_login or rules_user_logout" part of the should this be included?

Maybe if a site allowed both local and SAML authentication AND wanted something different to happen to just the SAML users, this might be useful... but I'm unaware of that use case at any organization interested in using this.

The bigger points are that getting this project to a stable, 2.0.0 release that requires a supported version of PHP and simpleSAMLphp library is important for security reasons. The previous D7 project never got beyond an alpha. The development resources to maintain this project in Backdrop, write test coverage, review feature requests, etc are very limited. Including code for Rules when no one is using simpleSAMLphp_auth with Rules, there is no testing to ensure it continues to work when dependencies are updated and responding to support requests for that feature would require a configuration no one involved is running is the type of unfocused approach that leads to an 11 year old project used by 9K+ sites only having an alpha2 release.

The authentication needs at Stanford and the University of Colorado are really well defined.

Doing less better is how we get from alpha2 to 2.0.0.

[joelsteidl]

I'm all about ripping out stuff we don't need. This shouldn't support rules in my opinion.

[argiepiano]

I'm not advocating one way or another. I was just responding to your question "I'm not sure the Rules integration in the module works with https://backdropcms.org/project/rules.". My answer was, yes, they do work.

And yes, probably the Rules event simplesamlphp_auth_rules_event_login is not really needed, as it can be handled by the user_login Rules event. simplesamlphp_auth_rules_event_register can be also handled by the rules-provided event user_insert