Skip to content

Conversation

@mtodd
Copy link
Member

@mtodd mtodd commented Aug 27, 2014

Looks like we're not properly handling recursive group membership checks. Opening this PR with a failing test to set the state for a fix.

@mtodd
Copy link
Member Author

mtodd commented Aug 28, 2014

Ran Group#group? tests separately to ensure that there was a real issue here. Confirmed: https://travis-ci.org/github/github-ldap/builds/33759563 fails on

assert @group.group?(object_classes.map(&:downcase))
when object classes are downcased, as is the default with Net::LDAP. So that was preventing recursion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Skips trying PosixGroup unless it's enabled. It's on by default, so this doesn't result in changed behavior, just means we can disable this behavior at will.

@MikeMcQuaid
Copy link
Contributor

This looks like a trivial and well-tested change that we need so merging.

MikeMcQuaid added a commit that referenced this pull request Aug 28, 2014
@MikeMcQuaid MikeMcQuaid merged commit d030fb8 into master Aug 28, 2014
@MikeMcQuaid MikeMcQuaid deleted the deeply-nested-membership-checks branch August 28, 2014 08:44
MikeMcQuaid added a commit that referenced this pull request Aug 28, 2014
So we can release #40 in a new GitHub Enterprise release.
MikeMcQuaid added a commit that referenced this pull request Aug 28, 2014
Because #40 warrants a new release by itself.
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.

4 participants