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

fixes possible override of uniqueMember by autodetection #15604

Merged
merged 2 commits into from
May 22, 2019

Conversation

blizzz
Copy link
Member

@blizzz blizzz commented May 17, 2019

  • uniqueMember was the default so we did not know whether this setting is
    desired or the initial value
  • autodetection of the user-group association attribute runs only when it
    was not set (as far as we knew)
  • the default is now empty
  • thus LDAPProvider might return this value as well (in exceptional cases)
  • if a group base is given (edge case), use this instead of general base
  • resolves group-user association attribute changed after click on "count groups" #12682

It changes behaviour a bit:

  • previously "uniqueMember" was the default and any LDAP config would work OOTB without hitting the test button after configuring groups. uniqueMember is not so common, and neither is not hitting "count groups". Imho, only impact for auto-installations were the attribute is not set specifically.
  • LDAPProvider can – in edge cases – return '' next to the known attributes

I would say it is still OK to backport… more to the rules is to keep for 17 only though. The impact is small and rather beneficial.

What's left

  • fix integration tests

* uniqueMember was the default so we did not know whether this setting is
  desired or the initial value
* autodetection of the user-group association attribute runs only when it
  was not set (as far as we knew)
* the default is now empty
* thus LDAPProvider might return this value as well (in exceptional cases)
* if a group base is given (edge case), use this instead of general base
* resolves #12682

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz blizzz added this to the Nextcloud 17 milestone May 17, 2019
@blizzz blizzz added 2. developing Work in progress and removed 3. to review Waiting for reviews labels May 20, 2019
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Code change makes sense 👍

Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

I could not test. But it makes some sense to me with my limited LDAP knowledge

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz blizzz added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels May 22, 2019
@blizzz
Copy link
Member Author

blizzz commented May 22, 2019

Still unsure about the backports. It's not the proper way.

@blizzz blizzz merged commit 7556118 into master May 22, 2019
@blizzz blizzz deleted the fix/12682/fix-possible-override-of-uniquemember branch May 22, 2019 17:09
@blizzz
Copy link
Member Author

blizzz commented May 23, 2019

Let's leave it for 17. I'll just respond it's fixed but not backportable since default values change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug feature: ldap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

group-user association attribute changed after click on "count groups"
3 participants