Skip to content

Properly nest groups when using memberOf to detect group membership, … - #18042

Merged
MorrisJobke merged 3 commits into
owncloud:masterfrom
GreenArchon:master
Aug 26, 2015
Merged

Properly nest groups when using memberOf to detect group membership, …#18042
MorrisJobke merged 3 commits into
owncloud:masterfrom
GreenArchon:master

Conversation

@GreenArchon

Copy link
Copy Markdown
Contributor

fixes #17759

@ghost

ghost commented Aug 4, 2015

Copy link
Copy Markdown

Thanks a lot for your contribution!
Contributions to the core repo require a signed contributors agreement http://owncloud.org/about/contributor-agreement/

Alternatively you can add a comment here where you state that this contribution is MIT licensed.

Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

@karlitschek

Copy link
Copy Markdown
Contributor

@blizzz

@GreenArchon

Copy link
Copy Markdown
Contributor Author

I hereby declare this diff MIT licensed

@LukasReschke

Copy link
Copy Markdown
Contributor

@owncloud-bot This is okay to test.

Comment thread apps/user_ldap/group_ldap.php Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

array &$seen?

@GreenArchon

Copy link
Copy Markdown
Contributor Author

Please re-review.

Also, keep in mind this comment from #17759 :

  • "This is not the best code I've written, as there's now 3 different functions in group_ldap that basically do the same thing (find elements based on attributes of a certain type on a DN), but they don't exactly return the same thing, and I didn't want to get into that for a quick bugfix"

Comment thread apps/user_ldap/group_ldap.php Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we can check for this setting before entering foreach uselessly and just return what we found.

@blizzz

blizzz commented Aug 7, 2015

Copy link
Copy Markdown
Contributor

Looks good to me, especially with adjusted Tests! Only minor aspects to improve found. When they are done, 👍 from me.

Then we need another tester / reviewer, maybe @tps800 @ryno83 @davitol

@blizzz blizzz added this to the 8.2-current milestone Aug 7, 2015
@blizzz

blizzz commented Aug 7, 2015

Copy link
Copy Markdown
Contributor

@karlitschek backport to 8.1 granted?

@karlitschek

Copy link
Copy Markdown
Contributor

yes. please backport :-)

@scrutinizer-notifier

Copy link
Copy Markdown

A new inspection was created.

@GreenArchon

Copy link
Copy Markdown
Contributor Author

@blizzz a55f233 is good for you?

@ghost

ghost commented Aug 12, 2015

Copy link
Copy Markdown

🚀 Test PASSed.🚀
chuck

@PVince81

Copy link
Copy Markdown
Contributor

@blizzz any update ? Merging this is blocking the 8.1.2 backport #18123

@MorrisJobke

Copy link
Copy Markdown
Contributor

👍

MorrisJobke added a commit that referenced this pull request Aug 26, 2015
Properly nest groups when using memberOf to detect group membership, …
@MorrisJobke
MorrisJobke merged commit 27af0e8 into owncloud:master Aug 26, 2015
@davitol

davitol commented Aug 31, 2015

Copy link
Copy Markdown
Contributor

👍 Tested with "version":"8.2.0.4","versionstring":"8.2 pre alpha"

@lock lock Bot locked as resolved and limited conversation to collaborators Aug 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8.1 - LDAP: Having memberOf enabled breaks nested groups

8 participants