Skip to content

Conversation

@butonic
Copy link
Member

@butonic butonic commented Sep 29, 2016

The uid might be empty if the mapped owncloud_username, eg. mail is empty. While admins should make sure the attribute is always set the ldap admins are only human. Errors happen and we need to protect against user errer. In this case we should die hard, giving everyone a hint at what might be wrong, which is why I use assert(). It will throw an exception and prevent further desaster. As in overwriting the quota for all users ...

@CLAassistant
Copy link

CLAassistant commented Sep 29, 2016

CLA assistant check
All committers have signed the CLA.

@mention-bot
Copy link

@butonic, thanks for your PR! By analyzing the annotation information on this pull request, we identified @blizzz, @DeepDiver1975 and @VicDeo to be potential reviewers

@butonic
Copy link
Member Author

butonic commented Sep 29, 2016

@owncloud/ldap quick review

@jvillafanez
Copy link
Member

🚫

Assertions can be disabled, and we should assume they will disabled on production. Taking into account that the code depends on an external service (LDAP) whose configuration is completely unknown, we can't consider that the code will be "safe" to be run without assertions once it reach production.

Just checking and throwing the exception seems a better option.

@butonic
Copy link
Member Author

butonic commented Sep 30, 2016

@jvillafanez @DeepDiver1975 done, makes sense.

@jvillafanez
Copy link
Member

👍

@DeepDiver1975 DeepDiver1975 merged commit 078372b into master Sep 30, 2016
@DeepDiver1975 DeepDiver1975 deleted the preventemptyuid branch September 30, 2016 16:05
@PVince81
Copy link
Contributor

PVince81 commented Oct 4, 2016

triple combo facepalm hit combo

@PVince81
Copy link
Contributor

PVince81 commented Dec 6, 2016

Please backport to 9.1 and 9.0

@jvillafanez
Copy link
Member

stable9.1: owncloud/core#26810
stable9: owncloud/core#26811

@SergioBertolinSG
Copy link
Contributor

This field corresponds in the ldap app's expert tab to uuid or internal username?

@INHack20 INHack20 mentioned this pull request Dec 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants