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

New LDAPProvider for user_ldap #23992

Closed
wants to merge 67 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@GitHubUser4234

GitHubUser4234 commented Apr 14, 2016

New LDAPProvider for user_ldap, please refer to #23855

@blizzz

This comment has been minimized.

Show comment
Hide comment
@blizzz

blizzz Apr 13, 2016

seems to be redundant with getAccess()

blizzz commented on apps/user_ldap/lib/backendutility.php in 3a4f0a1 Apr 13, 2016

seems to be redundant with getAccess()

@blizzz

This comment has been minimized.

Show comment
Hide comment
@blizzz

blizzz Apr 13, 2016

pass the UserManager (typehint against \OCP\IUserManager), this allows proper unit testing.

blizzz commented on apps/user_ldap/lib/ldapprovider.php in 3a4f0a1 Apr 13, 2016

pass the UserManager (typehint against \OCP\IUserManager), this allows proper unit testing.

@blizzz

This comment has been minimized.

Show comment
Hide comment
@blizzz

blizzz Apr 13, 2016

Also here, best to pass \OCP\ILogger in the constructor (when instantiating you get it from \OC::$server->getLogger()). Use this subsequently, it's far nicer than the old writeLog.

blizzz commented on apps/user_ldap/lib/ldapprovider.php in 3a4f0a1 Apr 13, 2016

Also here, best to pass \OCP\ILogger in the constructor (when instantiating you get it from \OC::$server->getLogger()). Use this subsequently, it's far nicer than the old writeLog.

@blizzz

This comment has been minimized.

Show comment
Hide comment
@blizzz

blizzz Apr 13, 2016

loginname does not help us here. You need to use Access::username2dn(), i.e. you need the Access instance here.

blizzz commented on apps/user_ldap/lib/ldapprovider.php in 3a4f0a1 Apr 13, 2016

loginname does not help us here. You need to use Access::username2dn(), i.e. you need the Access instance here.

@blizzz

This comment has been minimized.

Show comment
Hide comment
@blizzz

blizzz Apr 13, 2016

pls check indentation

blizzz commented on apps/user_ldap/user_ldap.php in 3a4f0a1 Apr 13, 2016

pls check indentation

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Apr 14, 2016

By analyzing the blame information on this pull request, we identified @MorrisJobke, @blizzz and @LukasReschke to be potential reviewers

mention-bot commented Apr 14, 2016

By analyzing the blame information on this pull request, we identified @MorrisJobke, @blizzz and @LukasReschke to be potential reviewers

@blizzz

This comment has been minimized.

Show comment
Hide comment
@blizzz

blizzz Apr 15, 2016

Contributor

Comments are inline, many taken out directly from my IDE ;) Smaller issues in general.

Three things remain, the first two low hanging fruits:

  • in user_ldap's appinfo/install.php and appinfo/update.php a check should happen wether a provider is configured. If not, write the config accordingly. Probably best to have a method in Helper that you call from both scripts.
  • In the LDAPProvider implementation actually check whether user_ldap is enabled, throw exception if not.
  • lib/public/ILDAPAccess as stated here https://github.com/owncloud/core/pull/23992/files#r59937276 I believe originally I was thinking of not returning an instance of the Access class here, but the pure LDAP Connection resource. However we have some normalization and conversion stuff in place in Access that you would need to take care of yourself if you have just the LDAP resource. We should figure out what you want to be able to do. We could have a shortcut to get the LDAP connection resource from Access->getConnection()->getConnectionResource(). To have proper control, some methods for controlling paged search, for instance, make sense, as do DNasBaseParameter() or sanitizeDN() and probably others. This needs some more thought work unfortunately.
Contributor

blizzz commented Apr 15, 2016

Comments are inline, many taken out directly from my IDE ;) Smaller issues in general.

Three things remain, the first two low hanging fruits:

  • in user_ldap's appinfo/install.php and appinfo/update.php a check should happen wether a provider is configured. If not, write the config accordingly. Probably best to have a method in Helper that you call from both scripts.
  • In the LDAPProvider implementation actually check whether user_ldap is enabled, throw exception if not.
  • lib/public/ILDAPAccess as stated here https://github.com/owncloud/core/pull/23992/files#r59937276 I believe originally I was thinking of not returning an instance of the Access class here, but the pure LDAP Connection resource. However we have some normalization and conversion stuff in place in Access that you would need to take care of yourself if you have just the LDAP resource. We should figure out what you want to be able to do. We could have a shortcut to get the LDAP connection resource from Access->getConnection()->getConnectionResource(). To have proper control, some methods for controlling paged search, for instance, make sense, as do DNasBaseParameter() or sanitizeDN() and probably others. This needs some more thought work unfortunately.
Show outdated Hide outdated 3rdparty
*
*/
$helper = new \OCA\User_LDAP\Helper();
$helper->setLDAPProvider();

This comment has been minimized.

@blizzz

blizzz Jul 8, 2016

Contributor

minor: there should be a blank line at the end of the file

@blizzz

blizzz Jul 8, 2016

Contributor

minor: there should be a blank line at the end of the file

GitHubUser4234 added some commits Jul 8, 2016

@GitHubUser4234

This comment has been minimized.

Show comment
Hide comment
@GitHubUser4234

GitHubUser4234 Jul 8, 2016

Thanks a lot @blizzz for your comments, they are implemented and committed. As discussed in IRC, the next step would be to implement unit tests to test all the public functions exposed by the LDAPProvider.

GitHubUser4234 commented Jul 8, 2016

Thanks a lot @blizzz for your comments, they are implemented and committed. As discussed in IRC, the next step would be to implement unit tests to test all the public functions exposed by the LDAPProvider.

@owncloud-bot

This comment has been minimized.

Show comment
Hide comment
@owncloud-bot

owncloud-bot Jul 22, 2016

Contributor

@GitHubUser4234

Thanks a lot for your contribution!
Contributions to the core repo require a signed contributors agreement http://owncloud.org/about/contributor-agreement/

Alternatively you can add a comment here where you state that this contribution is MIT licensed.

Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

Contributor

owncloud-bot commented Jul 22, 2016

@GitHubUser4234

Thanks a lot for your contribution!
Contributions to the core repo require a signed contributors agreement http://owncloud.org/about/contributor-agreement/

Alternatively you can add a comment here where you state that this contribution is MIT licensed.

Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

@elie195

This comment has been minimized.

Show comment
Hide comment
@elie195

elie195 Aug 24, 2016

Contributor

Is this PR targeted for implementation in 9.2? I'm looking forward to implementing it in my app to map UUID to owncloud_name in order to support LDAP in instances where the internal UUID mapping is set to the default.

Contributor

elie195 commented Aug 24, 2016

Is this PR targeted for implementation in 9.2? I'm looking forward to implementing it in my app to map UUID to owncloud_name in order to support LDAP in instances where the internal UUID mapping is set to the default.

@PVince81

This comment has been minimized.

Show comment
Hide comment
Member

PVince81 commented Sep 14, 2016

@jvillafanez

This comment has been minimized.

Show comment
Hide comment
@jvillafanez

jvillafanez Sep 14, 2016

Member

The changes seem big enough to be considered almost a new app, because I don't think it was originally planned the app contains such changes.

  • There are some refactor made there (sanitizeDN change the place in the PR) which is mixed with more changes. From my point of view, a bad idea since the change could have unexpected consecuences (some classes might be still calling to the old method). Such change should be isolated so we can verify that everything still works after the change.
  • Modifying lib/private/Server.php is a no-go from my point of view. An app such LDAP musn't modify core stuff. 🚫
  • Adding new interface to public library is also another no-go if it comes from another app such LDAP 🚫 . Core should provide generic interfaces for user and / or group handling, but never a specific interface for a specific service. It's fine is the LDAP app provide its own custom extension points so other apps hooks in there, but core shouldn't focus on LDAP.
Member

jvillafanez commented Sep 14, 2016

The changes seem big enough to be considered almost a new app, because I don't think it was originally planned the app contains such changes.

  • There are some refactor made there (sanitizeDN change the place in the PR) which is mixed with more changes. From my point of view, a bad idea since the change could have unexpected consecuences (some classes might be still calling to the old method). Such change should be isolated so we can verify that everything still works after the change.
  • Modifying lib/private/Server.php is a no-go from my point of view. An app such LDAP musn't modify core stuff. 🚫
  • Adding new interface to public library is also another no-go if it comes from another app such LDAP 🚫 . Core should provide generic interfaces for user and / or group handling, but never a specific interface for a specific service. It's fine is the LDAP app provide its own custom extension points so other apps hooks in there, but core shouldn't focus on LDAP.
@GitHubUser4234

This comment has been minimized.

Show comment
Hide comment
@GitHubUser4234

GitHubUser4234 Sep 14, 2016

I switched to Nextcloud and I don't have the resources to maintain this PR in ownCloud. Sorry for any inconvenience caused.

GitHubUser4234 commented Sep 14, 2016

I switched to Nextcloud and I don't have the resources to maintain this PR in ownCloud. Sorry for any inconvenience caused.

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