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

Escape commas, slightly differently #46

Merged
merged 1 commit into from
Oct 8, 2015

Conversation

jaredjennings
Copy link

This is a slightly different way from #38 to deal with escaped commas in Active Directory DNs, with tests added.

@domcleal
Copy link
Contributor

domcleal commented Oct 8, 2015

Thanks, looks fine to me. The only trouble is going to be the negative lookbehind, which isn't Ruby 1.8.7 compatible.

I see no reason for us to keep 1.8.7 compatibility in this gem any more, so let me sort that out first and then we'll be able to take this fix.

@domcleal
Copy link
Contributor

domcleal commented Oct 8, 2015

I've removed 1.8.7 from the test matrix in #47, so if you rebase you should pick this up and the tests should go green. Could you also please squash your commits to a single commit?

@jaredjennings
Copy link
Author

There remains one possibly important difference between this and #38: @brgerig, having split on a suitably unescaped comma, went on to unescape the commas in the base DN, and I have only unescaped the first attribute. I don't know yet which is the right way to do it.

@domcleal
Copy link
Contributor

domcleal commented Oct 8, 2015

I think not escaping it may be more correct. The escapes in the remainder of the DN are probably still valid as the base DN is still a DN, so needs appropriate escaping. The comma escapes when constructing a filter are the ones that are important to remove, since they'd no longer be needed in a filter expression.

@jaredjennings
Copy link
Author

OK, it looks like base should remain as it is, with commas escaped. I made an OU with a comma in the name, and a user and a group inside it; put the user in the group; specified the OU as the base_dn (for finding users) and as the group_dn, using a backslashed comma in each; and successfully found the user to be a member of the group, using Active Directory.

domcleal added a commit that referenced this pull request Oct 8, 2015
Escape commas, slightly differently
@domcleal domcleal merged commit d61ef75 into theforeman:master Oct 8, 2015
@domcleal
Copy link
Contributor

domcleal commented Oct 8, 2015

Great, thanks for the patch and re-testing!

@domcleal domcleal mentioned this pull request Oct 8, 2015
@jaredjennings
Copy link
Author

@jaredjennings jaredjennings deleted the commas-in-cn branch October 13, 2015 15:09
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.

None yet

2 participants