search

silverstripe / silverstripe-ldap

LDAP authentication support module for Silverstripe
BSD 3-Clause "New" or "Revised" License
3 stars 15 forks source link

Module is not compatible with silverstripe/mfa out of the box #25

Open robbieaverill opened 5 years ago

robbieaverill commented 5 years ago

Essentially a copy of https://github.com/silverstripe/silverstripe-activedirectory/issues/130 (SS3 issue in the equivalent repository).

The way that both LDAP and MFA are architected (by necessity) means that integrating MFA into LDAP would require customised project code (see https://github.com/silverstripe/silverstripe-mfa/issues/173 and https://github.com/silverstripe/silverstripe-mfa/pull/287).

We may want to look at a way to make this easier with SilverStripe 4. Perhaps putting more of the MFA logic into extensions might be an idea, where possible. This may also require some core changes.

The LDAP module (and the MFA module) might be able to make the PHP class hierarchy soup a little less salty by not overriding methods like this, but using Injector definitions to change the class names instead - however, that would be global, and would not only apply for specific authenticators, so achieving this might also require some changes in core to do something like "if there's a specific LoginForm configured for [current authenticator name] then use it, otherwise use the default LoginForm implementation".

Anyway, raising here as a placeholder issue - not sure what will be actioned from this yet.

ScopeyNZ commented 5 years ago

I commented briefly in the MFA module about this (https://github.com/silverstripe/silverstripe-mfa/issues/173#issuecomment-515246063)

I think we can add some code here to provide support OOTB with zero configuration. It's not really possible with core architecture (I think) right now to build it in a way where they can combine seamlessly (with zero overlap).