Skip to content
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

make Get(Raw)AttributeValues compare case insensitive #111

Merged
merged 4 commits into from
Jun 11, 2020

Conversation

vetinari
Copy link
Contributor

@vetinari vetinari commented May 4, 2017

This is the same way an LDAP server compares during a search request.

Fixes #109

@trdyer
Copy link

trdyer commented Aug 11, 2017

would love to have this!

func TestGetAttributeValue(t *testing.T) {
dn := "testDN"
attributes := map[string][]string{
"Alpha": {"value"},
Copy link
Contributor

Choose a reason for hiding this comment

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

what should happen if you have {"alpha":{"a","b"}, "ALPHA": {"c", "d"}} and you search for "Alpha"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is that possible? isn't the attribute name case insensitive?

Copy link
Member

Choose a reason for hiding this comment

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

Shall we at least add a test case with attribute names differing only in case and document via the tests how we'd handle it? That way at least it's more obvious.

@johnweldon
Copy link
Member

I'd like to see the case sensitivity to be toggled by a package variable (var CaseInsensitive = true maybe?) that defaults to insensitive searching, but allows the library consumer to toggle that behaviour if backward compatibility requires it.

@liggitt
Copy link
Contributor

liggitt commented Sep 30, 2017

I still think it's a mistake to compress the value space by default.

Explicitly case-insensitive lookup functions seem clearer to me. Would also need to define the behavior for that (does it prefer an exact matching name over a case insensitive name match that comes first in order? Does it aggregate values specified for names that differ only by case?)

@vetinari vetinari force-pushed the lc-get-attr-values branch from fe017e8 to 757d81e Compare October 3, 2017 12:07
@johnweldon
Copy link
Member

If you're willing to add parallel functions that are explicitly case-insensitive (see discussion on #129) I think we can get this merged in sooner. If you're willing to take on that extra work I'd appreciate it @vetinari

these are the case insensitive counter parts, e.g.
  GetAttributeValue <=> GetEqualFoldAttributeValue
using strings.EqualFold to compare the attrribute names

fixes go-ldap#109 go-ldap#129
@vetinari vetinari force-pushed the lc-get-attr-values branch from 2481b09 to 9c998c4 Compare June 7, 2018 05:45
@vetinari
Copy link
Contributor Author

vetinari commented Jun 7, 2018

rebased against 99171d7 and squashed

@vetinari
Copy link
Contributor Author

vetinari commented Jun 8, 2018

@johnweldon @liggitt can we merge this?

srenatus added a commit to srenatus/dex that referenced this pull request Jun 19, 2018
LDAP itself doesn't match attributes case-sensitively, so a query for

    (fOO=bar)

will return all the entries having `foo: bar`, regardless of how the
attribute name is defined in the schema.

However, the result entries we get will have the case according to what
the server returned. So, assuming the schema calls it `foo`, this is
what we're given.

Now, for certain attributes that are user-configurable -- NameAttr for
example -- this can cause trouble: the query returns the right results,
but the LDAP connector can't make sense of it as it's looking for an
attribute called `fOO`.

The real life case underlying this conundrum is Active Directory's
sAMAccountName.

Note that this is a bit of a workaround. Ideally, we'd use
`(*ldap.Entry).GetAttributes(string) []string` to access the attributes;
but until the case sensitivity issue is fixed there[1], changing the way
we read the results doesn't help us.

[1]: go-ldap/ldap#111

Signed-off-by: Stephan Renatus <[email protected]>
@ricardofandrade
Copy link

I'd like to have this fix as well. Any ETA for merging it?

@srenatus
Copy link

@liggitt 🏓 Any news regarding this? 😃

func TestGetAttributeValue(t *testing.T) {
dn := "testDN"
attributes := map[string][]string{
"Alpha": {"value"},
Copy link
Member

Choose a reason for hiding this comment

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

Shall we at least add a test case with attribute names differing only in case and document via the tests how we'd handle it? That way at least it's more obvious.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get(Raw)AttributeValues should compare attributes case insensitive
6 participants