caelum / mamute

Q&A Engine
http://www.mamute.org/
Apache License 2.0
337 stars 152 forks source link

Mamute should support LDAP #44

Closed monitorjbl closed 10 years ago

monitorjbl commented 10 years ago

For those wanting to use Mamute internally, the ability to use LDAP is pretty important. Users definitely don't want to have to sign up to use the app if they've already got a known user/pass for their organization.

There should be an optional feature that, when activated, allows users to log into the system using their LDAP credentials. The Mamute User profile should be created on the first login, and the user should not have to create an account. The account creation UI should be disabled entirely while this feature is active.

monitorjbl commented 10 years ago

Added a basic implementation for LDAP integration. Very simple for the moment, it only allows user authentication; all authorization is still done by Mamute. On first login, a User object is created in the same fashion as SignupController.

There is an additional, implicit requirement in using a credential store like LDAP to do authentication: user sign up must not be possible. As a simple fix for this, I've added an additional parameter to disable signup. This parameter is included in DefaultViewObjects and used on loginForm.jsp and header.jspf to disable rendering of the signup form.

monitorjbl commented 10 years ago

Based on feedback from @csokol, the AuthController needs to be refactored into two classes and to use the @EnvironmentDependent annotation: LDAPAuthController and DbAuthController. I think this makes sense, and I started looking at implementing it.

Turns out there's some unfortunate complications around this. There should to be a common reference point so things like header.jspf can refer to it in their linkTos. I tried making AuthController an abstract class and adding the two child classes. Sadly, it looks like vRaptor doesn't recognize abstract classes in doing the linkTo operation :(

Is there a way to customize vRaptor to allow this so there doesn't have to be a bunch of if-else's in the JSP files?

monitorjbl commented 10 years ago

Any pointers? Not really sure how to do the whole abstract pattern in vRaptor.

csokol commented 10 years ago

I think we can try to customize VRaptor's linkto to find out the route from the base class. We'll need to override the DefaultLinker class. I can try to guide you in this customization or do it myself, do you have any code with this refactor to send?

csokol commented 10 years ago

Sorry, this is the class we'll need to override: https://github.com/caelum/vraptor4/blob/master/vraptor-core/src/main/java/br/com/caelum/vraptor/view/Linker.java

Basically, what we need to do is to get the actual class from the base (abstract) class (I think we can do it with cdi), I don't know if it will be easy, but I think it's possible.

monitorjbl commented 10 years ago

Ah, interesting, I'll take a look. Thanks for the pointer!

csokol commented 10 years ago

If you want to send an early PR, we can try to guide you with this...

Chico Sokol

On Tue, Sep 2, 2014 at 3:42 PM, monitorjbl notifications@github.com wrote:

Ah, interesting, I'll take a look. Thanks for the pointer!

— Reply to this email directly or view it on GitHub https://github.com/caelum/mamute/issues/44#issuecomment-54198152.

monitorjbl commented 10 years ago

Sure thing, I've been holding off on the first commit till I figured out how to support linkTo Should hopefully have a first-pass PR very soon

monitorjbl commented 10 years ago

Abstract classes never get passed to Linker or LinkToHandler. Stepping through the code, it looks like its because the EL expression isn't able to find the class. Something else in vRaptor determines what classes are able to be controllers with Weld's reflection, and it skips over interfaces and abstract classes. These classes aren't even passed to EnvironmentDependentControllerHandler.

I'm still new to vRaptor, but it doesn't seem like it's able to support the interface-implementor pattern without a lot of work. From what I can tell, there's other problems that will arise even if this worked. For instance, logic to handle Result#redirectTo would need to exist for stuff like this.

Is there another pattern that would work better here with vRaptor?

csokol commented 10 years ago

The problem is that we are not including the abstract class into servlet context, as the default controller handler does: https://github.com/caelum/vraptor4/blob/master/vraptor-core/src/main/java/br/com/caelum/vraptor/ioc/ControllerHandler.java#L74

We can try to do this to fix linkTo issues but we still would need customize the component that performs the redirect...

A simpler solution would be to put the auth route in messages.properties and read it to render our links. What do you think? Do you want to keep trying with the customization of vraptor approach or go with this simpler alternative?

Chico Sokol

On Tue, Sep 2, 2014 at 6:19 PM, monitorjbl notifications@github.com wrote:

Abstract classes never get passed to Linker or LinkToHandler. Stepping through the code, it looks like its because the EL expression isn't able to find the class. Something else in vRaptor determines what classes are able to be controllers with Weld's reflection, and it skips over interfaces and abstract classes. These classes aren't even passed to EnvironmentDependentControllerHandler.

I'm still new to vRaptor, but it doesn't seem like it's able to support the interface-implementor pattern without a lot of work. From what I can tell, there's other problems that will arise even if this worked. For instance, logic to handle Result#redirectTo would need to exist for stuff like this https://github.com/caelum/mamute/blob/master/src/main/java/org/mamute/brutauth/auth/handlers/LoggedHandler.java#L30 .

Is there another pattern that would work better here with vRaptor?

— Reply to this email directly or view it on GitHub https://github.com/caelum/mamute/issues/44#issuecomment-54219431.

monitorjbl commented 10 years ago

Yeah, I'd considered doing something like that based on what @leocwolter did for the Solr stuff. It would work from the JSPs, but the internal code would probably have issues. Result#redirectTo seems to let you redirect with parameters and everything.

Maybe a better route would be to make DefaultAuthenticator feature-aware somehow and leave the AuthController in place...there could be an interface that DbAuthenticator and LdapAuthenticator implement and the CDI does some ~magic~ to inject the right class for the feature.

csokol commented 10 years ago

Good idea! We can extract an interface for the authenticator and create a producer to build the right implementation depending on the environment!

Chico Sokol

On Tue, Sep 2, 2014 at 6:52 PM, monitorjbl notifications@github.com wrote:

Yeah, I'd considered doing something like that based on what @leocwolter https://github.com/leocwolter did for the Solr stuff. It would work from the JSPs, but the internal code would probably have issues. redirectTo seems to let you redirect with parameters and everything.

Maybe a better route would be to make DefaultAuthenticator feature-aware somehow and leave the AuthController in place...there could be an interface that DbAuthenticator and LdapAuthenticator implement and the CDI does some ~magic~ to inject the right class for the feature.

— Reply to this email directly or view it on GitHub https://github.com/caelum/mamute/issues/44#issuecomment-54223451.

monitorjbl commented 10 years ago

Sweet, I'll just make a basic Authenticator producer right now to figure out which class to return. In the future, if there are a bunch of authenticators it could probably be made to use a classpath scanner.

monitorjbl commented 10 years ago

I think this issue can be closed now. I'm not promising that the feature will work on everyone's LDAP at this point, but from here on out it would probably be best to deal with future problems as separate issues.

csokol commented 10 years ago

You're right, closing!

@monitorjbl could you please answer this question in meta.mamute? http://meta.mamute.org/291-ldap-authentication

Then your answer would work as a documentation of this feature (maybe we should link it in our readme's FAQ) :-)

monitorjbl commented 10 years ago

I don't have access to answer questions apparently :(

monitorjbl commented 10 years ago

I think I need to have 10 reputation to do anything but ask questions on that site. Somebody upvote my posts!

leocwolter commented 10 years ago

Actually you need 100 reputation to interact with old questions:

2014-09-10 18:38 GMT-03:00 monitorjbl notifications@github.com:

I think I need to have 10 reputation to do anything but ask questions on that site. Somebody upvote my posts!

— Reply to this email directly or view it on GitHub https://github.com/caelum/mamute/issues/44#issuecomment-55187096.

leocwolter commented 10 years ago

I interacted with it for you, can you try now?

monitorjbl commented 10 years ago

Yep, that worked. Thanks!