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

Enable auth failures based on 'groupfilter' #495

Closed

Conversation

acquireflyingjohns
Copy link

We noticed that valid LDAP users could authenticate even if they weren't a member of the group specified by the groupfilter in local.php. The presence of valid 'grouptree' and 'groupfilter' configs in local.php would have no bearing on the login if the user's password is accepted.

It's possible that we have a misconfiguration in dokuwiki that is causing this behavior, but I couldn't determine from the authldap documentation that this was the case. I also could not see in master's authldap/auth.php where the login is denied based on membership in the groupfilter's group, so I added code to checkPass and getUserData to implement this check.

I hope this helps. Let me know if you have any questions.

… authentication if the bound user is not a member of the group specified by the groupfilter.
@splitbrain
Copy link
Collaborator

The group filter wasn't really intended to deny access to the wiki. It is used to determine which LDAP object define a group associated with a certain user. Access limitation was supposed to be done via ACLs then. Your patch would not allow in any user without a (matching) group and tell them that the username or password was wrong when in fact they were perfectly correct.

However the ability to completely lock out people based on their group membership has often be requested. Your patch would achieve that, though in a rather crude way (lying to the user). I'm undecided if I like it.

@Chris--S
Copy link
Collaborator

I'm not too familiar with LDAP, but like splitbrain says, are we getting group filter to do two things?
If so would a second var, restrictedgroups or allowedgroups, be more appropriate.


// We've authenticated the user, now verify group membership if groupfilter was specified
if($this->getConf('groupfilter')){
if(!$info['satisfygroupfilter']){
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this simply be:

return $info['satisfygroupfilter'];

@splitbrain
Copy link
Collaborator

see also https://bugs.dokuwiki.org/index.php?do=details&task_id=2350 (for a previous similar request)

@Klap-in
Copy link
Collaborator

Klap-in commented Feb 25, 2014

@TyIsI
Copy link

TyIsI commented Feb 25, 2014

As the poster/submitter of FS#2104, I'd love to see this issue resolved. Need any help implementing this?

@Klap-in
Copy link
Collaborator

Klap-in commented Mar 18, 2014

(currently this branch needs that the master branch is merged into it, to resolve conflicts)

It is a often requested feature, so any help is welcome :) @acquireflyingjohns how about improving this PR?

@splitbrain
Copy link
Collaborator

The more I think about the more I think this should not be implemented in the AD and LDAP plugins. Denying access based on group membership is not really dependent on the used auth backend. It should be implemented as a general feature. And that could easily be done in a dfferent plugin IMHO.

@ponsfrilus
Copy link

@acquireflyingjohns finally I did succeed to limit the access by using the user filter with specifying the group and removing additional group* stuff in the config file.

Here is my configuration:

$conf['plugin']['authldap']['server'] = 'ldap://ldap.xxxx.ch';
$conf['plugin']['authldap']['port'] = 636;
$conf['plugin']['authldap']['usertree'] = 'c=ch';
$conf['plugin']['authldap']['userfilter'] = '(&(uid=%{user})(objectClass=posixAccount)(memberof=GROUPNAME))';
$conf['plugin']['authldap']['version'] = 3;
$conf['plugin']['authldap']['starttls'] = 1;
$conf['plugin']['authldap']['debug'] = 0;

The LDAP filter "(&(uid=%{user})(objectClass=posixAccount)(memberof=GROUPNAME))" just allows members of the group to login on the wiki, and avoid sub queries on the LDAP. At least for me it was the simplest and smater solution, without additional plugins or code modification.

Hope it helps...

@TyIsI
Copy link

TyIsI commented Mar 18, 2014

@splitbrain I like your thinking! It's been a while since I last looked at the code, but I think the only challenge would be how to handle plugin dependencies? Because there would still need to be some sort of adjustment in the actual back-end query and on large scale systems, this could generate a bit of extra load. Maybe use a uniform way to specify the requirements, with a backend dependent implementation?

That being said, @ponsfrilus his solution is quite effective as well. (For LDAP specifically.)

@splitbrain
Copy link
Collaborator

@Chris--S not sure I understand what you mean. I would let the user login, then intercept with a action plugin and logout the user right away if the groups don't match (with displaying a message).

@Chris--S
Copy link
Collaborator

@splitbrain that works. At the time I was suggesting - rather than doubling up on the groupfilter setting meaning - adding an extra config setting to the auth plugins which could be used to restrict login access to users who belong to at least one of the groups listed in the new setting.

However, is this really a message/notification issue?
ACLs can deny access based on group membership. If after logging in a user has no access to any part of the wiki, that could have a specific message different from the message if they only have no access to the current page.

@splitbrain
Copy link
Collaborator

For some reason (I do not really understand) some people don't want people to even login. I'd argue let them login and just deny them any rights through ACL. But still some people seem to want something different. For them a plugin as I outlined should work. So I vote for closing this PR.

@Chris--S
Copy link
Collaborator

Ok.

@splitbrain splitbrain closed this Mar 18, 2014
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

6 participants