Skip to content
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

added checking for loginldap #1076

Closed
wants to merge 1 commit into from
Closed

added checking for loginldap #1076

wants to merge 1 commit into from

Conversation

gitirabassi
Copy link

when loginldap is present

@AdamEttenberger
Copy link

This only appears to addresses a symptom of the problem.
Have you been able to sign into /admin with LDAP credentials ?

@gitirabassi
Copy link
Author

Still not able to login with ldap but if this fix is not implemented there is a deadlock with login and loginldap and there is no way to further test loginldap.

@AdamEttenberger
Copy link

AdamEttenberger commented Apr 25, 2017

I've created a pull request for the loginldap which allows you to create the admin.* permissions in loginldap.yaml,
markleary/grav-plugin-loginldap#9

I've also been taking a stab at this problem, and have the bare minimum functionality for logging into the admin panel with LDAP credentials.
It's kind of a dirty work around, forcing the admin login form to use the loginldap Controller when loginldap is enabled. Still learning how Grav does things like cross-plugin communication.

Since the admin panel has so many assumptions about the user object being from a yaml file, many user related things just don't work quite yet.
Also, it looks like you can't use the update feature in the admin panel for admin while using loginldap rather than login because of a dependency list somewhere.

Feel free to take a look :
https://github.com/AdamEttenberger/grav-plugin-admin/commit/917e29c47200912bf02776db87d030459976538c

@flaviocopes
Copy link
Contributor

Admin is currently tight with the Login plugin, and being able to find users in the user/accounts folder.

Also, LoginLDAP instead of being a complete substitute for the Login plugin should hook in some way into it, providing an external source for credentials, much like the login-oauth plugin does.
Then, Admin should be refactored to use more general methods provided by the Login plugin.

We cannot really merge this PR like it is now, as Admin is not going to work without the Login plugin installed, right now.

@markleary
Copy link

I (author of loginldap plugin) did originally attempt to hook into Login as login-oauth does but ran into a few problems which led me to write it as a drop-in replacement instead. Unfortunately, I can't recall what the specific issues were - but architecturally it did not make a lot of sense as loginldap is providing the full stack rather than just authentication as is the case with oauth. I also wanted to avoid caching user information locally in user/accounts.

Personally, I do not use the admin plugin, so this is a non-issue for me. If I have some time in the next few days I'll take a look and see if I can figure something out on my end. Hooking into the login plugin may be the best approach but I suspect it will also require some refactoring of login as well.

@katgirl
Copy link

katgirl commented Jul 5, 2017

i will login in admin by local user and in frontend by ldapuser so i need this pull too

@Ahuahuachi
Copy link

Hello, just posting to ask if there is any plan on making the admin plugin work with loginldap still.

@rhukster
Copy link
Member

rhukster commented Nov 8, 2017

Too be honest, i'm not 100% sure this even still works. It's a 3rd party plugin and probably a better solution would be to have a hook that sidesteps this check that can be used by 3rd party plugins, rather than hardcoding the plugin name directly.

So no, we need a better solution than this.

Copy link
Member

@rhukster rhukster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a better solution than hardcoding more plugins. I'm pretty sure login plugin is needed in more places than just the actual login. A better approach is to use the new login events to hook into ldap rather than replacing the whole login plugin.

@rhukster
Copy link
Member

rhukster commented Dec 5, 2017

We're making some progress on this. The next release of login will have the events we need. Then we just need to change admin plugin to use the standard login process and hence the events. So baby steps!

@rhukster
Copy link
Member

LDAP login plugin using standard login events has been developed for a client, and will be released as free plugin shortly.

@rhukster rhukster closed this May 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants