On Fri, Jul 12, 2013 at 02:50:26PM +0200, Branko Čibej wrote:
> On 12.07.2013 13:54, 刘新星 wrote:
> > Thank you for your suggestions!
> >
> > mod_ldap for performance improvement is a further consideration.
> >
> > Actually I want to add ldap support for mod_authz_svn most.
> > As far as I can see, when we need ldap authentication, we would take
> > these combination 'apache + subversioin'.
> > Is it a good idea to move ldap.c from libsvn_subr to mod_authz_svn ?
I think we should ensure that both svnserve and mod_authz_svn interpret
the authz rule configuration file in the same way. If one supports
ldap and the other does not, the authz rules files are not compatible
between server implementations. Users should be able to switch between
apache and svnserve without such problems.
> First of all, there should be no ldap.c in Subversion. If you want to
> use LDAP, use the apr-util LDAP API. See:
>
> http://svn.apache.org/viewvc/apr/apr-util/branches/1.5.x/include/apr_ldap.h.in?view=markup
Ah, that's neat! I was looking for exactly this sort of thing before
writing my initial reply to the patch submission, and didn't find an
LDAP API. But I only checked APR-util-1.4, not the 1.5 version.
So, yes, that should be used to run ldap queries from svnserve.
> >From the above it follows that libsvn_subr is not the right place to put
> LDAP support. That can be, at best, a server feature, so you're basiclly
> looking at adding it to svnserve and nowhere else, using the LDAP
> support provided by apr-util.
>
> I am strongly against the idea of adding LDAP support to mod_authz_svn.
> There is already a mod_ldap, it doesn't make sense to duplicate
> functionality. If mod_ldap has performance problems -- well then, that's
> the place to solve them. It's open source after all.
I think you misread what we were saying. mod_ldap caches ldap replies,
and should perform better than the proposed patch, which does no caching.
> Adding /group/ support to mod_authz_svn is completely orthogonal to
> LDAP. Let's not mix the two issues. And frankly, I'd rather spend time
> adding proper group- and role-based authorization to the repository than
> heaping more stuff onto the current config-file-based authz layer.
Please, let's not tie the "we need a new filesystem" discussion into
this tiny feature addition that solves someones problem.
I'd bet that if we're waiting for a new filesystem for this, nothing will
happen for the next two years at least. A working patch speaks louder
than a great idea in this case. There's no harm done by adding this.
It does not stand in the way of a new filesystem implementation.
In other words, if you object to the proposed patch on the basis that
you'd rather do this in the filesystem, I first expect to see a working
patch from you that does just that :)
Received on 2013-07-12 15:09:29 CEST