[Ldap] Allow search scoping #20310

xunto · 2016-10-26T16:14:38Z

Q	A
Branch?	"master"
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	-

Quite trivial, PR adds "scope" attribute to the query options. For example:

    public function ldapAction()
    {
        $ldap = $this->get("app.ldap");
        $dn = $this->getParameter("dn");

        $results = $ldap->query($dn, "(objectClass=*)", [
            "scope" => Query::SCOPE_ONELEVEL
        ])->execute();

        foreach ($results as $entry) {
            var_dump($entry->getDn()); echo "<br/>";
        }

        return new Response("");
    }

SCOPE_BASE: http://php.net/manual/ru/function.ldap-read.php
SCOPE_ONELEVEL: http://php.net/manual/ru/function.ldap-list.php
SCOPE_SUBTREE: http://php.net/manual/ru/function.ldap-search.php

xunto · 2016-10-27T10:51:00Z

I need to further tell that without this change ldap is almost unusable in some cases.

For example, to build a tree of departments, you need to search in SCOPE_ONELEVEL but you can't (without this PR).

My dirty workaround is to look at length of entry's dn to understand if this is sub-department (cn is uuid, so it has fixed size). This is very dirty and unreliable solution.

xunto · 2016-11-01T17:52:43Z

Why does nobody look at this PR (it's week already)? @csarrazi marked as author in https://github.com/symfony/ldap. Should I contact him?

csarrazi · 2016-11-01T21:10:53Z

@xunto I do not receive notifications unless I am explicitly tagged in a comment.

csarrazi

Just change the constant values to strings instead of integers, and this is LGTM. I've considered doing this a while ago, but it got out of my mind. Thanks @xunto!

csarrazi · 2016-11-01T21:26:59Z

src/Symfony/Component/Ldap/Adapter/QueryInterface.php

@@ -23,6 +23,10 @@
    const DEREF_FINDING = 0x02;
    const DEREF_ALWAYS = 0x03;

+    const SCOPE_BASE = 0x00;


Do not use integer constants here unless they are actually part of the specification. Here, please use string values.

I.e.

const SCOPE_BASE = 'base'; const SCOPE_ONELEVEL = 'one_level'; const SCOPE_SUBTREE = 'subtree';

csarrazi · 2016-11-01T21:29:25Z

One thing though, please also add functional tests for the scope handling.

csarrazi · 2016-11-02T12:56:35Z

src/Symfony/Component/Ldap/Adapter/ExtLdap/Query.php

+                    $func = 'ldap_list';
+                    break;
+                case static::SCOPE_SUBTREE:
+                default:


The default should actually throw an exception (which should actually never be thrown, as the OptionsResolver is actually the one which sanitizes the scope option) instead of being a fall-through for the subtree scope.

@csarrazi actually I already defined allowed values in option OptionsResolver (c6c47ae#diff-822ec15d626e08f71c9ebaea383b5a89R40). So default is only for linter.

Okay. So throw an exception in the default, and set $func to ldap_search in the SCOPE_SUBTREE case. :)

csarrazi · 2016-11-02T12:56:44Z

src/Symfony/Component/Ldap/Tests/Adapter/ExtLdap/AdapterTest.php

+            'scope' => Query::SCOPE_SUBTREE,
+        ))->execute()->count();
+
+        $this->assertNotEquals($one_level, $subtree);


I do not feel that this tests anything useful (Apart from checking that a one level scope is different from a subtree). We should actually check whether we have the expected content in the output from the test.

I'm working on it right now)

csarrazi · 2016-11-02T12:57:36Z

src/Symfony/Component/Ldap/Tests/Fixtures/data/fixtures.ldif

+
+dn: cn=Scope,cn=Ldap,ou=Components,dc=symfony,dc=com
+objectclass: organizationalunit
+cn: Scopes


Add newline to end of file.

xunto · 2016-11-02T13:11:20Z

I apologise for big amount of commits. My distro have different paths for openldap so I can't run tests on my computer.

csarrazi · 2016-11-02T13:34:22Z

@xunto No problem, as long as you squash everything later! ;)

You could also use the csarrazi/symfony-ldap docker repository, if you want to easily spawn an openldap instance, btw!

xunto · 2016-11-02T13:37:53Z

@csarrazi Everything should be ok now. Thanks for review. I going to squash it right now.

csarrazi · 2016-11-02T13:38:12Z

You're welcome!

xunto · 2016-11-02T13:40:34Z

@csarrazi done

xunto · 2016-11-02T13:42:44Z

Oh, didn't noticed yout anwser about default in switch/case. Give me a minute...

xunto · 2016-11-02T14:27:50Z

@csarrazi Everything is done. Tests pass.

csarrazi · 2016-11-02T21:33:24Z

👍

xunto · 2016-11-03T08:34:23Z

@csarrazi Should I slap deciders or mergers now?

csarrazi · 2016-11-03T08:51:36Z

Yup! :)

xunto · 2016-11-03T09:04:57Z

@csarrazi Thanks for help! @symfony/deciders, it's your turn!

fabpot · 2016-11-03T15:19:17Z

Just for your information, this new feature won't be merged for 3.2, but considered for 3.3.

xunto · 2016-11-03T16:14:36Z

@fabpot Sad but ok, I think.

So it won't get into the master until 3.2 is released? So, in december.

fabpot · 2016-11-03T16:20:14Z

Right, that's correct.

xabbuh · 2016-11-05T11:23:00Z

👍

csarrazi · 2016-11-13T14:31:04Z

Hey @xunto! Sorry for getting back one more time. Could you just amend your PR and change the scope names so they respect RFC 2255? ("base" / "one" / "sub").

See https://tools.ietf.org/html/rfc2255

The scope construct is used to specify the scope of the search to
perform in the given LDAP server. The allowable scopes are "base"
for a base object search, "one" for a one-level search, or "sub" for
a subtree search. If scope is omitted, a scope of "base" is assumed.

That would be great! :)

xunto · 2016-11-14T02:09:28Z

@csarrazi Ok, will do.

xunto · 2016-11-15T13:33:57Z

@csarrazi done.

xunto · 2016-12-02T06:34:28Z

@symfony/deciders, 3.2 is released. Looks like now we can merge this.

fabpot · 2016-12-04T13:44:31Z

Thank you @xunto.

This PR was squashed before being merged into the 3.3-dev branch (closes #20310). Discussion ---------- [Ldap] Allow search scoping | Q | A | | --- | --- | | Branch? | "master" | | Bug fix? | no | | New feature? | yes | | BC breaks? | no | | Deprecations? | no | | Tests pass? | yes | | Fixed tickets | - | | License | MIT | | Doc PR | - | Quite trivial, PR adds "scope" attribute to the query options. For example: ```php public function ldapAction() { $ldap = $this->get("app.ldap"); $dn = $this->getParameter("dn"); $results = $ldap->query($dn, "(objectClass=*)", [ "scope" => Query::SCOPE_ONELEVEL ])->execute(); foreach ($results as $entry) { var_dump($entry->getDn()); echo "<br/>"; } return new Response(""); } ``` SCOPE_BASE: http://php.net/manual/ru/function.ldap-read.php SCOPE_ONELEVEL: http://php.net/manual/ru/function.ldap-list.php SCOPE_SUBTREE: http://php.net/manual/ru/function.ldap-search.php Commits ------- 83c7915 [Ldap] Allow search scoping

carsonbot added Status: Needs Review Ldap Feature labels Oct 26, 2016

csarrazi reviewed Nov 1, 2016

View reviewed changes

csarrazi reviewed Nov 2, 2016

View reviewed changes

[Ldap] Allow search scoping

9f83a2b

javiereguiluz added this to the 3.3 milestone Nov 6, 2016

Fix scope names

2b085b9

fabpot closed this Dec 4, 2016

javiereguiluz mentioned this pull request Dec 23, 2016

[Ldap] Allow search scoping symfony/symfony-docs#7284

Closed

nicolas-grekas modified the milestones: 3.x, 3.3 Mar 24, 2017

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

guilliamxavier mentioned this pull request Sep 13, 2018

ldap query minor refactoring #28459

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Ldap] Allow search scoping #20310

[Ldap] Allow search scoping #20310

xunto commented Oct 26, 2016 •

edited by javiereguiluz

Loading

xunto commented Oct 27, 2016 •

edited

Loading

xunto commented Nov 1, 2016 •

edited

Loading

csarrazi commented Nov 1, 2016

csarrazi left a comment

csarrazi Nov 1, 2016

csarrazi commented Nov 1, 2016

csarrazi Nov 2, 2016

xunto Nov 2, 2016 •

edited

Loading

csarrazi Nov 2, 2016 •

edited

Loading

csarrazi Nov 2, 2016

xunto Nov 2, 2016

csarrazi Nov 2, 2016

xunto commented Nov 2, 2016 •

edited

Loading

csarrazi commented Nov 2, 2016

xunto commented Nov 2, 2016

csarrazi commented Nov 2, 2016

xunto commented Nov 2, 2016

xunto commented Nov 2, 2016

xunto commented Nov 2, 2016

csarrazi commented Nov 2, 2016

xunto commented Nov 3, 2016

csarrazi commented Nov 3, 2016

xunto commented Nov 3, 2016

fabpot commented Nov 3, 2016

xunto commented Nov 3, 2016 •

edited

Loading

fabpot commented Nov 3, 2016

xabbuh commented Nov 5, 2016

csarrazi commented Nov 13, 2016

xunto commented Nov 14, 2016

xunto commented Nov 15, 2016

xunto commented Dec 2, 2016

fabpot commented Dec 4, 2016

[Ldap] Allow search scoping #20310

[Ldap] Allow search scoping #20310

Conversation

xunto commented Oct 26, 2016 • edited by javiereguiluz Loading

xunto commented Oct 27, 2016 • edited Loading

xunto commented Nov 1, 2016 • edited Loading

csarrazi commented Nov 1, 2016

csarrazi left a comment

Choose a reason for hiding this comment

csarrazi Nov 1, 2016

Choose a reason for hiding this comment

csarrazi commented Nov 1, 2016

csarrazi Nov 2, 2016

Choose a reason for hiding this comment

xunto Nov 2, 2016 • edited Loading

Choose a reason for hiding this comment

csarrazi Nov 2, 2016 • edited Loading

Choose a reason for hiding this comment

csarrazi Nov 2, 2016

Choose a reason for hiding this comment

xunto Nov 2, 2016

Choose a reason for hiding this comment

csarrazi Nov 2, 2016

Choose a reason for hiding this comment

xunto commented Nov 2, 2016 • edited Loading

csarrazi commented Nov 2, 2016

xunto commented Nov 2, 2016

csarrazi commented Nov 2, 2016

xunto commented Nov 2, 2016

xunto commented Nov 2, 2016

xunto commented Nov 2, 2016

csarrazi commented Nov 2, 2016

xunto commented Nov 3, 2016

csarrazi commented Nov 3, 2016

xunto commented Nov 3, 2016

fabpot commented Nov 3, 2016

xunto commented Nov 3, 2016 • edited Loading

fabpot commented Nov 3, 2016

xabbuh commented Nov 5, 2016

csarrazi commented Nov 13, 2016

xunto commented Nov 14, 2016

xunto commented Nov 15, 2016

xunto commented Dec 2, 2016

fabpot commented Dec 4, 2016

xunto commented Oct 26, 2016 •

edited by javiereguiluz

Loading

xunto commented Oct 27, 2016 •

edited

Loading

xunto commented Nov 1, 2016 •

edited

Loading

xunto Nov 2, 2016 •

edited

Loading

csarrazi Nov 2, 2016 •

edited

Loading

xunto commented Nov 2, 2016 •

edited

Loading

xunto commented Nov 3, 2016 •

edited

Loading