Create docs to describe lack of compatibility with LDAP

[brynwhyman]

It is possible for a site to use shared authentication services for a user:

• LDAP (a supported module)
• activedirectory (in limited support for SS3)
• SAML (not a supported module but still used in projects)

There will be additional work required for a site to implement MFA to be compatible with LDAP. Docs will be created to describe the limitations and guidance for proceeding. This work will not be done as part of the MFA development initiative.

Actions

• Docs are included in the MFA module
• Raise an issue on LDAP to highlight the incompatibility

Pull requests

• SS3: DOCS Add hypothetical guide to integrating MFA into another authenticator e.g. LDAP #286
• SS4: DOCS Add hypothetical guide to integrating MFA into another authenticator e.g. LDAP #287

Upstream issues raised

• Module is not compatible with silverstripe/mfa out of the box silverstripe-activedirectory#130
• Module is not compatible with silverstripe/mfa out of the box silverstripe-ldap#25

[ScopeyNZ]

High level assumption: this would require additional code in the modules to support it. It definitely would be additional effort though.

[brynwhyman]

Noting that there are only two stacks running the silverstripe/ldap module on our infrastructure.

[indygriffiths]

For what it's worth both Dashboards (SS3) use only the activedirectory module as the authenticator (LDAPAuthenticator).

[brynwhyman]

Thanks Indy, I was going to ask about that! Added it to the list above for reference.

[robbieaverill]

PRs at #286 and #287. Since the implementations differ slightly between SilverStripe 3 and 4 I've made two PRs instead.

Raise an issue on LDAP to highlight the incompatibility

I've created one in each repository (activedirectory and LDAP). Does that mean we can close #160 now, which is basically the same as the ones I just created?

Just noting that I have a feeling we'd have to made adjustments to framework in order to make this achievable out of the box, because I don't think it's extensible enough yet - this is a hunch though, not a fact.

TL;DR: making MFA work out of the box with other authenticators (such as LDAP) is achievable, but it would require a little bit of re-architecture in the MFA module and probably a bit of tweaking in core.

Here a couple of notes about this:

• The way we've architected the MFA module is that it has a MemberAuthenticator which replaces the core MemberAuthenticator.
• In MFA's case I'm not convinced that is necessary, since it doesn't really care what happens with the email and password - it's only concerned with adjusting the authentication flow after that happens.
  • In the case of the LDAP module, it does need to be architected this way because it replaces the authentication logic (with email and password) entirely.
  • The fact that both modules extend the core MemberAuthenticator means that naturally MFA will never work out of the box with LDAP because they are essentially rival implementations of the same architecture.
• A quick look at the MemberAuthenticator in MFA shows that all it does is change the default implementations of LoginHandler and ChangePasswordHandler. This could easily be done with Injector configuration instead, which would remove the need for MFA to "be" a MemberAuthenticator, but you'd then end up at the next problem;
• The same way that MFA and LDAP work with Form implementations is to extend the core Form instances. This has the same issue as the MemberAuthenticator mentioned above.
  • There are only a couple of methods in MFA's LoginHandler which actually overload the core logic: doLogin(), redirectAfterSuccessfulLogin(), and getBackURL().
  • MFA's ChangePasswordHandler only overloads one: changepassword().
  • In theory, everything other than these methods could be shifted into an Extension instead, which would allow LDAP to inherit the MFA logic automatically since it would be in an Extension applied to LDAP's LoginForm parent class.
  • We may need to adjust the core logic a little to ensure that we can replace the overloaded methods mentioned above with extension hooks instead.
• I don't see a reason why this couldn't be achieved in a minor release (both for MFA and for framework).
• I'm taking exclusively about SilverStripe 4 here - SilverStripe 3 is what it is, we've documented a path to plug things together for it, but we won't be improving that version's architecture.

[ScopeyNZ]

• The way we've architected the MFA module is that it has a MemberAuthenticator which replaces the core MemberAuthenticator.
• In MFA's case I'm not convinced that is necessary, since it doesn't really care what happens with the email and password - it's only concerned with adjusting the authentication flow after that happens.

There's one important clarification here. We do actually come in just before the end. The email/password is verified but we jump in the authentication flow just before the log in actually occurs.

We could implement a custom request handler for all our MFA endpoints to use instead of the LoginHandler itself - that could extract a lot of the logic out of LoginHandler. I think we've done pretty well putting it in traits though. I imagine (apart from LDAP/AR) most authenticators won't want to integrate with MFA. Authenticators like oAuth would have no need to (for instance).

---

I think we should just add the code you mentioned in your SS4 docs (about LDAP) into the LDAP module. We can put some of that class_exists hacky-ness in and we can do some "if module exists" config to auto-wire it. We could also switch to overriding LoginHandler with Injector instead of providing our own MemberAuthenticator. I feel like this is a pretty awkward way of overriding compared to replacing the default authenticator though. It just feels less intuitive.

[ScopeyNZ]

I've merged the two docs PRs. This is good discussion, but the issue has been addressed so I'll close this. Discussion can continue on the two new issues raised (see OP).