Problem/Motivation

Modify ldap_authorization/src/Plugin/authorization/Provider/LDAPAuthorizationProvider.php

Steps to reproduce

LDAP query:
/(?<=cn=).+?[^\,]*/i

query string:

CN=xxx-yyyyy,OU=Groups,OU=example,DC=example,DC=example,DC=example,DC=com

If I need to take the value of the first OU, I can't do it.
/(?<=OU=).+?[^\,]*/i

CN=xxx-yyyyy,OU=MyGroups,OU=example,DC=example,DC=example,DC=example,DC=com
CommentFileSizeAuthor
#10 3187788-10-fail-test.patch864 bytesdravenk
#7 3187788-fail-test.patch834 bytesdravenk

Issue fork ldap-3187788

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

dravenk created an issue. See original summary.

dravenk opened merge request !10

dravenk’s picture

dravenk
He/Him
Primary language English
commented
Status: Active » Needs review
dravenk’s picture

dravenk
He/Him
Primary language English
commented
Issue summary: View changes
grahl’s picture

grahl
he/him
Location Zürich
commented
Status: Needs review » Needs work

Hi

Thanks for the bug report, the patch itself looks fine, but we have a test already covering that logic and I'd like to have that case covered, since we seem to have made assumptions in LdapAuthorizationProviderTest::testFilterProposal() which don't cover this case.

@dravenk: Could you supply the additional test case in there in your patch?

dravenk’s picture

dravenk
He/Him
Primary language English
commented
Assigned: Unassigned » dravenk

Thank you. I will submit a fail test patch.

dravenk’s picture

dravenk
He/Him
Primary language English
commented
StatusFileSize
new 3187788-fail-test.patch834 bytes
dravenk’s picture

dravenk
He/Him
Primary language English
commented
Status: Needs work » Needs review

The latest MR contains a failed test for #7, so it will pass.

dravenk’s picture

dravenk
He/Him
Primary language English
commented
Assigned: dravenk » Unassigned
Status: Needs review » Reviewed & tested by the community

The latest MR adds only one test. Since #5 has been reviewed, I changed the status to RTBC.

dravenk’s picture

dravenk
He/Him
Primary language English
commented
StatusFileSize
new 3187788-10-fail-test.patch864 bytes

I reexamined the code and found that using assertEquals is more accurate. This is because the regular expression provided here will filter out only the first group content, avoiding the second group content.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: 3187788-10-fail-test.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

dravenk’s picture

dravenk
He/Him
Primary language English
commented
Status: Needs work » Reviewed & tested by the community

The test case is given in comment #comment-13958122 is clear.

  • grahl committed 8f2f14d on 8.x-4.x authored by dravenk 
    Issue #3187788 by dravenk: When LDAP query has only one value, the...
grahl’s picture

grahl
he/him
Location Zürich
commented
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

Thanks!

I made the count comparison strict but otherwise committed as-is.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.