Add LDAP Authentication

[ynsta]

Hello,

Here a new PR for ldap authentication on your latest branch and without the config part.

All settings are given via environment vars.

I also added a docker-compose as an example with all env vars and a postgresql.

Regards,

[deanishe]

I'm not so sure about the benefit of LDAP auth. It doesn't really strike me as a good fit for a "simple bookmark manager". What's the use case?

Regarding the code itself:

1. Please don't vendor dependencies.
2. Adding an env package seems like overkill just for three functions that are only used in auth. At any rate, I think env.GetEnv* stutters rather. Could the functions not be named env.Get* (or just env.String() etc.)?
3. You're calling NewLDAPAuth() in init() and silently ignoring any error. Please don't do that.
4. Is "thrusted certificate" an LDAP term, or is LDAPSettings.ThrustedCertificates a typo?

[ynsta]

Hello,

Sorry for the late reply I was kind of busy on other subjects.

I'll update my PR to address your comments.

For the use case LDAP is mandatory for me I don't want my users to have to have yet another password as there is already a central authentication server. Also when I have a new user or one that leave, I only need to change at one place for all apps.

Regards,

[deanishe]

For the use case LDAP is mandatory for me

I mean the use case for Shiori users in general.

[ynsta]

Yes I understood, but I might not be the only user that want to use a bookmark manager in a professional environment with a ldap server or more generally a AD server.

[deanishe]

I might not be the only user that want to use a bookmark manager in a professional environment with a ldap server or more generally a AD server.

You might not be. But I'd be surprised if there are many others, as Shiori really isn't targeted at such environments. It doesn't even have "full" multi-user support (there's only one collection of bookmarks).

In face of that, it seems at least premature to integrate 3rd-party authentication, especially heavy-duty enterprise solutions.

If I accept the PR, it's a feature I have to support, and I know next to nothing about LDAP, let alone have an LDAP environment to develop Shiori in.

That's a big ask, and one I'm strongly inclined to reject in face of the risk/effort vs the apparently limited benefit to Shiori users as a whole.

Could this not be replaced by a simpler, backend-agnostic interface to allow the use of external auth providers? Something that would be (a) simpler to maintain in Shiori and (b) more broadly useable for typical, self-hosting, non-enterprise Shiori users?

[jedrzejowski]

First, I really like idea of one centralized bookmark database, it's fits good for enterprise needs.
However I tried using your PR, but it failed in my case with following error appearing every time logging occurs:

shiori_1  | LDAP: not found (unable to read LDAP response packet: unexpected EOF, unable to read LDAP response packet: unexpected EOF)

OpenLDAP server logs this:

conn=1980 fd=22 ACCEPT from IP=192.168.X.Y:45210 (IP=0.0.0.0:636)
TLS: can't accept: An unexpected TLS packet was received..

I have tried many things like updating dependiecies, skiping cert vertifaction, but none of them worked.

[jedrzejowski]

Ok, i found solution. There is a difference between github.com/go-ldap/ldap and github.com/go-ldap/ldap/v3.
I made fix 17f0be2, but I am not going to do PR. I believe it is not good enough but it may help someone else.

[ynsta]

I'll try to re-base and rework this feature soon if some people are interested in it. My use case is to have a same database for a whole team to share our technical watch.

[symgryph]

I am personally of the opinion that we should probably just start a new fork. There are PR’s from last year that still haven’t even been looked at. I had quite a few of my own but I just withdrew out of frustration. Thomas J Munn

…

On Mar 3, 2021, at 11:26, Stany MARCEL ***@***.***> wrote:  I'll try to re-base and rework this feature soon if some people are interested in it. My use case is to have a same database for a whole team to share our technical watch. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[fmartingr]

Hey @ynsta, thank you for your commitment to this, as I said in #491, we can't support this at the moment with the current state of Shiori. Please read my comment there, if you have any questions or want to move the conversation forward please do! I would like to properly allow multiple auth methods in Shiori but the bases are not there yet. Hope you understand!