Implementation of trustExternal() method in authldap plugin #205

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
@brenard

brenard commented Mar 26, 2013

It's an implementation of trustExternal() method of authldap plugin.

I had an configuration parameter to enable/disable this feature.

It's useful when, for example, you use Apache capability to authenticate user and you only want to retrieve his informations from LDAP (name, mail, groups, ...).

lib/plugins/authldap/auth.php
@@ -36,6 +36,10 @@ public function __construct() {
return;
}
+ if ($this->getConf('external')) {

This comment has been minimized.

@selfthinker

selfthinker Apr 7, 2013

Collaborator

Minor issue: Some indention is missing here.

@selfthinker

selfthinker Apr 7, 2013

Collaborator

Minor issue: Some indention is missing here.

This comment has been minimized.

@brenard

brenard Apr 7, 2013

Fixed in my second commit.

@brenard

brenard Apr 7, 2013

Fixed in my second commit.

lib/plugins/authldap/auth.php
@@ -36,6 +36,10 @@ public function __construct() {
return;
}
+ if ($this->getConf('external')) {
+ $this->cando['external'] = true;

This comment has been minimized.

@splitbrain

splitbrain Apr 7, 2013

Owner

shouldn't cando['logout'] be also set to false in this case?

@splitbrain

splitbrain Apr 7, 2013

Owner

shouldn't cando['logout'] be also set to false in this case?

This comment has been minimized.

@brenard

brenard Apr 7, 2013

Yes, you'r right, logoff haven't sense in this case. I read the IRC discussion and I think you understand my goal. This solution is very useful when using Apache with mod_cas for instance to have SSO authentification and retrieve other user's informations from LDAP.

@brenard

brenard Apr 7, 2013

Yes, you'r right, logoff haven't sense in this case. I read the IRC discussion and I think you understand my goal. This solution is very useful when using Apache with mod_cas for instance to have SSO authentification and retrieve other user's informations from LDAP.

@splitbrain

This comment has been minimized.

Show comment
Hide comment
@brenard

This comment has been minimized.

Show comment
Hide comment
@brenard

brenard May 17, 2013

Hello,

Do you have any suggestion about this feature to permit merge ? I use it in production since two month now, and it work very well.

Thank's

brenard commented May 17, 2013

Hello,

Do you have any suggestion about this feature to permit merge ? I use it in production since two month now, and it work very well.

Thank's

@selfthinker

This comment has been minimized.

Show comment
Hide comment
@selfthinker

selfthinker Jun 2, 2013

Collaborator

We weren't considering any new features, because we were in feature freeze. We discuss pull requests every 2 weeks (but only bug-related ones during feature freeze) and have started to discuss old PRs today, but ran out of time to go through all of them. That means you will get feedback in 2 weeks at the latest.

Collaborator

selfthinker commented Jun 2, 2013

We weren't considering any new features, because we were in feature freeze. We discuss pull requests every 2 weeks (but only bug-related ones during feature freeze) and have started to discuss old PRs today, but ran out of time to go through all of them. That means you will get feedback in 2 weeks at the latest.

@brenard

This comment has been minimized.

Show comment
Hide comment
@brenard

brenard Jun 2, 2013

Ok, it's clear now :) Thank's for this explanation.

brenard commented Jun 2, 2013

Ok, it's clear now :) Thank's for this explanation.

@michitux

This comment has been minimized.

Show comment
Hide comment
@michitux

michitux Jun 16, 2013

Collaborator

As DokuWiki uses the "u" GET-parameter instead of the username that's supplied by the web server this means that any user which gets through the external authentication can switch to any other user account which is most probably not wanted. The code should at least check if $INPUT->bool('http_credentials') is false.

As this doesn't really depend on LDAP but could actually be used with any auth backend (that would only be responsible for managing the user details but not the password) and a simple http authentication it might be better to implement this as separate auth plugin that uses another (configurable) auth plugin for loading/changing/... the user data. The plugin could simply forward all requests to the other auth plugin and only handle trustExternal().

Collaborator

michitux commented Jun 16, 2013

As DokuWiki uses the "u" GET-parameter instead of the username that's supplied by the web server this means that any user which gets through the external authentication can switch to any other user account which is most probably not wanted. The code should at least check if $INPUT->bool('http_credentials') is false.

As this doesn't really depend on LDAP but could actually be used with any auth backend (that would only be responsible for managing the user details but not the password) and a simple http authentication it might be better to implement this as separate auth plugin that uses another (configurable) auth plugin for loading/changing/... the user data. The plugin could simply forward all requests to the other auth plugin and only handle trustExternal().

@splitbrain

This comment has been minimized.

Show comment
Hide comment
@splitbrain

splitbrain Jun 16, 2013

Owner

I agree with @michitux

Owner

splitbrain commented Jun 16, 2013

I agree with @michitux

@splitbrain splitbrain closed this Jun 16, 2013

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