georchestra / georchestra-gateway

GNU General Public License v3.0
0 stars 6 forks source link

Adding Header authentication to Gateway (Apache) #58

Closed marwanehcine closed 11 months ago

marwanehcine commented 1 year ago

On this PR, we add a new filter which allows to authenticate users using the HTTP headers received from Apache. A new LDAP account is then created if isCreateNonExistingUsersInLDAP property is true.

This feature is similar to the security-proxy's "SP trust SP" one here: https://github.com/georchestra/georchestra/blob/master/security-proxy/src/main/java/org/georchestra/security/ProxyTrustAnotherProxy.java

marwanehcine commented 11 months ago

Hi, I’m seeing a bunch of ldap related imports on classes that have nothing to do with ldap. We need to maintain certain level of decoupling. Think of the ldap components as if they were a plugin on a separate project module. The classes in core can’t depend on the plugin, it’s to be the other way around. Moreover, this will break if ldap is disabled (I’m pretty sure we can disable it in the configuration). Sorry for not being more specific but I’m on mobile and my laptop decided to not boot the UI. But you can easily check, if a class is importing ldap components and it’s not in an ldap package then it’s wrong. You can define or extend existing interfaces to allow for the implementation of the ldap-specific logic on the ldap “plugin” or subsystem though.

Hello @groldan , @pmauduit , @fvanderbiest , I was investigating this issue, code here is same as "GatewaySecurityConfiguration" where we have introduced account creation on Gateway. For me the question is : should we allow ldap account creation at gateway level ? I thought the response was yes. But what was missing is to make dependency optional (whether there is ldap sever configured or not) To solve this issue, I have added "LdapEnabledCondition" which check if there is at least one ldap server with status enabled. I have also used this condition when creating beans related to LDAP in GatewaySecurityConfiguration class. Please check this commit : https://github.com/georchestra/georchestra-gateway/pull/58/commits/f17a823f0fdf42177a71d5f127a1afea4dd9ee71

Thanks,

groldan commented 11 months ago

TODO

groldan commented 11 months ago

Closing as superseded by #63