Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Ldap] Allow search scoping#20310

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
xunto wants to merge2 commits intosymfony:masterfromxunto:ldap_scoping
Closed

[Ldap] Allow search scoping#20310

xunto wants to merge2 commits intosymfony:masterfromxunto:ldap_scoping

Conversation

@xunto
Copy link
Contributor

@xuntoxunto commentedOct 26, 2016•
edited by javiereguiluz
Loading

QA
Branch?"master"
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets-
LicenseMIT
Doc PR-

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

publicfunctionldapAction()    {$ldap =$this->get("app.ldap");$dn =$this->getParameter("dn");$results =$ldap->query($dn,"(objectClass=*)", ["scope" => Query::SCOPE_ONELEVEL        ])->execute();foreach ($resultsas$entry) {var_dump($entry->getDn());echo"<br/>";        }returnnewResponse("");    }

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

csarrazi reacted with thumbs up emoji
@xunto
Copy link
ContributorAuthor

xunto commentedOct 27, 2016•
edited
Loading

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
Copy link
ContributorAuthor

xunto commentedNov 1, 2016•
edited
Loading

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

@csarrazi
Copy link
Contributor

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

Copy link
Contributor

@csarrazicsarrazi left a comment

Choose a reason for hiding this comment

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

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!

constDEREF_FINDING =0x02;
constDEREF_ALWAYS =0x03;

constSCOPE_BASE =0x00;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

I.e.

constSCOPE_BASE ='base';constSCOPE_ONELEVEL ='one_level';constSCOPE_SUBTREE ='subtree';

@csarrazi
Copy link
Contributor

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

$func ='ldap_list';
break;
casestatic::SCOPE_SUBTREE:
default:
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
ContributorAuthor

@xuntoxuntoNov 2, 2016•
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@csarrazicsarraziNov 2, 2016•
edited
Loading

Choose a reason for hiding this comment

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

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

'scope' => Query::SCOPE_SUBTREE,
))->execute()->count();

$this->assertNotEquals($one_level,$subtree);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I'm working on it right now)


dn: cn=Scope,cn=Ldap,ou=Components,dc=symfony,dc=com
objectclass: organizationalunit
cn: Scopes
Copy link
Contributor

Choose a reason for hiding this comment

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

Add newline to end of file.

@xunto
Copy link
ContributorAuthor

xunto commentedNov 2, 2016•
edited
Loading

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

@csarrazi
Copy link
Contributor

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

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

@xunto
Copy link
ContributorAuthor

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

@csarrazi
Copy link
Contributor

You're welcome!

@xunto
Copy link
ContributorAuthor

@csarrazi done

@xunto
Copy link
ContributorAuthor

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

@xunto
Copy link
ContributorAuthor

@csarrazi Everything is done. Tests pass.

@csarrazi
Copy link
Contributor

👍

@xunto
Copy link
ContributorAuthor

@csarrazi Should I slap deciders or mergers now?

@csarrazi
Copy link
Contributor

Yup! :)

@xunto
Copy link
ContributorAuthor

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

@fabpot
Copy link
Member

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

@xunto
Copy link
ContributorAuthor

xunto commentedNov 3, 2016•
edited
Loading

@fabpot Sad but ok, I think.

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

@fabpot
Copy link
Member

Right, that's correct.

@xabbuh
Copy link
Member

👍

@javiereguiluzjaviereguiluz added this to the3.3 milestoneNov 6, 2016
@csarrazi
Copy link
Contributor

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").

Seehttps://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
Copy link
ContributorAuthor

@csarrazi Ok, will do.

@xunto
Copy link
ContributorAuthor

@csarrazi done.

@xunto
Copy link
ContributorAuthor

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

@fabpot
Copy link
Member

Thank you@xunto.

@fabpotfabpot closed thisDec 4, 2016
fabpot added a commit that referenced this pull requestDec 4, 2016
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.phpSCOPE_ONELEVEL:http://php.net/manual/ru/function.ldap-list.phpSCOPE_SUBTREE:http://php.net/manual/ru/function.ldap-search.phpCommits-------83c7915 [Ldap] Allow search scoping
@nicolas-grekasnicolas-grekas modified the milestones:3.x,3.3Mar 24, 2017
@fabpotfabpot mentioned this pull requestMay 1, 2017
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

1 more reviewer

@csarrazicsarrazicsarrazi left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

3.3

Development

Successfully merging this pull request may close these issues.

7 participants

@xunto@csarrazi@fabpot@xabbuh@javiereguiluz@nicolas-grekas@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp