New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Add LDAP authentication #3263

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
10 participants
@immae
Copy link

immae commented Jul 5, 2017

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? no
Documentation wallabag/doc#31
Translation no
Fixed tickets #966
License MIT

This PR adds LDAP authentication to wallabag.
It is still a work in progress.

  • Need to find a way to make it optional (in particular not making ldap extension mandatory if the user doesn't need it)
  • Need to write some tests
  • Need to write some documentation

Features:

  • Find, create and update users based on LDAP informations
  • Handles both kinds of users (non-LDAP and LDAP)
  • Optionnally adds admin role based on LDAP information
  • Works with the API too
@j0k3r

This comment has been minimized.

Copy link
Member

j0k3r commented Jul 6, 2017

Looks interesting!
Do not forget to run php bin/php-cs-fixer fix src/

@tcitworld

This comment has been minimized.

Copy link
Member

tcitworld commented Jul 9, 2017

in particular not making ldap extension mandatory if the user doesn't need it

It shouldn't be required in the composer.json file but checked when the LdapHydrator is used.

@tcitworld

This comment has been minimized.

Copy link
Member

tcitworld commented Jul 9, 2017

Ccing some YunoHost wallabag 2 package contributors here @jeromelebleu @JimboJoe @lapineige

@immae

This comment has been minimized.

Copy link
Author

immae commented Jul 9, 2017

@tcitworld : the problem I have is not removing it from composer.json, but handling the rest of the configuration. The LdapHydrator happens too late in the process

@tcitworld

This comment has been minimized.

Copy link
Member

tcitworld commented Jul 9, 2017

You may add a listener to the kernel.request event and check it here, but it may be bad in terms of performance.

@lapineige

This comment has been minimized.

Copy link
Collaborator

lapineige commented Jul 9, 2017

Thanks @tcitworld :)
It would indeed be really cool to integrate that in our wallabag v2 package :)

@immae

This comment has been minimized.

Copy link
Author

immae commented Jul 9, 2017

@tcitworld I spent a few days trying to learn more of the internal ways of symfony (especially the kernel). Now I "just have" to write it correctly so that it happens at "compilation" time, but it's doable :)

edit: Yes, kernel.request is way too late to do that for performance

@j0k3r

This comment has been minimized.

Copy link
Member

j0k3r commented Jul 9, 2017

Maybe you can hook inside the WallabagUserExtension?

@immae

This comment has been minimized.

Copy link
Author

immae commented Jul 11, 2017

Ok, now LDAP is completely optional:

  • If you want to use ldap, you need to "composer require fr3d/ldap-bundle"
  • Otherwise, no ldap is needed (not even the php extension)

@j0k3r : That's what I was trying to do, but services and parameters have different priority rules so not everything can happen at load time.

@j0k3r

This comment has been minimized.

Copy link
Member

j0k3r commented Jul 11, 2017

If you want to use ldap, you need to "composer require fr3d/ldap-bundle"

This'll be a no go for the final user.
Composer might fail to run in the final user server because it'll need more RAM, etc.

We should provide a package with the bundle in it and a single option to enable ldap during installation (for example in parameters.yml) or not. This should be a dead simple as possible for the final user.

@immae

This comment has been minimized.

Copy link
Author

immae commented Jul 11, 2017

@j0k3r : it's not possible, the fr3d/ldap-bundle requires php ldap extension. Unless you want to make it mandatory for everyone, even people who don't want ldap ?

@immae

This comment has been minimized.

Copy link
Author

immae commented Jul 11, 2017

@j0k3r that was my main problem in the last days: making ldap extension not mandatory if the user doesn't need it, so that someone who needs ldap can simply activate it on demand

@immae

This comment has been minimized.

Copy link
Author

immae commented Jul 11, 2017

Note that the "composer require fr3d/ldap-bundle" can be integrated in the "make install" script if necessary, which already contains composer commands.

Would that be suitable for you?

@immae

This comment has been minimized.

Copy link
Author

immae commented Jul 11, 2017

I changed the install script so that it can be enabled simply with:

make install LDAP_ENABLED=true

plus updating configuration in parameters.yml

@immae immae referenced this pull request Jul 11, 2017

Closed

Add LDAP documentation #31

@immae

This comment has been minimized.

Copy link
Author

immae commented Jul 11, 2017

documenation added

@nicosomb

This comment has been minimized.

Copy link
Member

nicosomb commented Jul 11, 2017

I think that we need a separated bundle for this feature, like I did for https://github.com/nicosomb/wallabag-carbondate-bundle and https://github.com/nicosomb/wallabag-camo-bundle.

@immae

This comment has been minimized.

Copy link
Author

immae commented Jul 12, 2017

@nicosomb I cannot qualify that as a "simple" setup...

I’m sorry, but I’m not willing to spent more time on that. This PR works and I'm already using is successfuly in production. I made it "dead simple as possible for the final user" as required, as well as making it completely optional. I’m okay to do some work to add unit and functional tests, but if the rest doesn’t suit you I’ll just close the PR and let someone else do it the right way.

@immae immae closed this Jul 15, 2017

@immae immae deleted the immae:ldap branch Jul 15, 2017

@ViViDboarder

This comment has been minimized.

Copy link

ViViDboarder commented Oct 28, 2017

It seems a shame to throw away a pr for a 3 year old feature request.

Is there guidance as to why some features should be separate bundles vs included? It seems like many features and integration are included by default. Is there something that makes this a candidate for exclusion? I don't see anything in the developer documentation as to when or how to make external bundles...

@immae

This comment has been minimized.

Copy link
Author

immae commented Oct 28, 2017

ViViDboarder: As far as I know, they don’t really care.

But you can just cherry-pick the patch and use it as is, I use it successfully for months now without any issue.

@almet

This comment has been minimized.

Copy link

almet commented Nov 1, 2017

Hi everyone!

First and more importantly, thanks @immae for taking the time to write this. Don't be mistaken: this is really appreciated!

Now, what we need is to find an acceptable way to get this merged upstream. My understanding of the issue is that the fact this code relies on a php-module makes it hard to have in the core. I can see two outcomes for it:

  • Package it as a separate plugin, as @nicosomb suggested. It probably makes it harder to install for the end user but relaxes the expectations to merge this in wallabag directly;
  • Detect if the module is enabled, and if it's the case propose an option to enable LDAP support in the settings.

Wallabag team: would option 2 be an acceptable approach?

@ViViDboarder

This comment has been minimized.

Copy link

ViViDboarder commented Nov 2, 2017

I have very little (read: no) experience with PHP, however if there is any kind of guidance as to how one would bundle it, I'd take a stab.

However, I am having trouble understanding why. It seemed like the original objection was that it was too burdensome on the users to run composer require fr3d/ldap-bundle before installing. However that seems to be counter to the bundle methodology since all those bundles also seem to require an additional command plus additional configuration.

This probably stems from me having no clue how Symphony works or the design guidelines for Wallabag. Symphony seems huge, so not sure where a relevant place to begin would be and the developer docs for Wallabag seem pretty sparse.

Like I said, happy to help, but would appreciate a point in the right direction!

@DMW007

This comment has been minimized.

Copy link

DMW007 commented Mar 31, 2018

Any news on this? Wallabag is a nice piece of software, but would be the only app without LDAP on my server. This prevents me from using it in production. I saw feature requests for LDAP that were created years ago. It would be nice if we can find a solution for this issue.

@immae

This comment has been minimized.

Copy link
Author

immae commented Jun 16, 2018

Hey there,
Since the discussion seems impossible at the moment, but I still get people contacting me to do something to help having LDAP in wallabag, I created a branch on a fork of this repository, containing the ldap feature.

It’s currently rebased above 2.3.2 (last version), and I’ll try to follow up when there are new releases:

https://git.immae.eu/?p=github/wallabag/wallabag.git;a=shortlog;h=refs/heads/gitolite_local/ldap

Hope it will be useful.

@joshp23

This comment has been minimized.

Copy link
Contributor

joshp23 commented Nov 13, 2018

Just some input. It seems appropriate to simply bundle this in and allow the admin to switch LDAP on or off in the GUI. Am I missing something about this that makes that a no-go?

@j0k3r

This comment has been minimized.

Copy link
Member

j0k3r commented Nov 14, 2018

@immae

This comment has been minimized.

Copy link
Author

immae commented Nov 14, 2018

I updated the branch on my fork, it’s now rebased above 2.4

@j0k3r

This comment has been minimized.

Copy link
Member

j0k3r commented Jan 24, 2019

Just to let you know that we might publish a new bundle to enable LDAP on wallabag.
It'll require some manual step to install it (and for sure won't work on Raspberry because of lack of ram).
We'll publish it under the wallabag organisation.
We'll let you know when it's available.

@ismaelbouyaf

This comment has been minimized.

Copy link

ismaelbouyaf commented Jan 24, 2019

Thanks for the news @j0k3r . In the meantime, my branch https://git.immae.eu/?p=github/wallabag/wallabag.git;a=shortlog;h=refs/heads/gitolite_local/ldap is rebased on top of last release (2.3.6), and requires less RAM to prepare: I added the lines in the composer.lock / composer.json, which was the main problem of the previous method (The composer require don’t need to be done on the target host, it can be done at buildtime)

Someone asked me yesterday help to enable ldap on their instance, I found out it was not as trivial as I thought and detailed steps here: https://forum.chatons.org/t/wallabag-authentification-avec-ldap/241/2 (in French)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment