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

Check if app is enabled instead of if class exists #18872

Merged
merged 1 commit into from
Sep 7, 2015

Conversation

RobinMcCorkell
Copy link
Member

Further down in the same file, we already use isEnabled() to check if an app is enabled. This approach is better not only for consistency, but also because it avoids autoloading the class.

In addition, the existing check was bogus, since the class would get autoloaded regardless of if LDAP is enabled or not (hooray insecure autoloading!). Since the merge of #18839, that was prevented, so user management broke. This fixes #18871

@nickvergessen Hopefully you see how the other PR actually found this completely broken code, and that's a good thing?

cc @icewind1991 @DeepDiver1975 @MorrisJobke @PVince81

@scrutinizer-notifier
Copy link

A new inspection was created.

@nickvergessen
Copy link
Contributor

👎 revert all that stuff please and make a proper PR

@LukasReschke
Copy link
Member

👍

@MorrisJobke
Copy link
Contributor

👎 revert all that stuff please and make a proper PR

Beside the problems with the old PR, this is nevertheless useful and also works without the changes to the autoloader.

@LukasReschke
Copy link
Member

The autoloader PR increased ownCloud's overall security against a specific kind of attack and I'd prefer to have this in 8.2. - We can easily fix such regressions.

@oparoz
Copy link
Contributor

oparoz commented Sep 7, 2015

I'm not denying the benefits of all the recent autoloading improvements, but having to work on apps while tracking core master is a major pain already and merged non-BC PRs without a heads up make it way worse, sucking bandwidth off other things which needs to get done. I'm sure it's not the last autoloading PR we'll see.

ownCloud's development process owner needs to ask the process managers to design something new so that changes belonging to that non-BC category are only merged after specific acceptance criteria have been met and if it's blocking other work, then this needs to belong to a sprint in order to be resolved quicker.

@MorrisJobke
Copy link
Contributor

Tested and works 👍

This PR fixes a problem. The autoloading thingy needs to be discussed in a separate pr.

MorrisJobke added a commit that referenced this pull request Sep 7, 2015
Check if app is enabled instead of if class exists
@MorrisJobke MorrisJobke merged commit a35bc97 into master Sep 7, 2015
@MorrisJobke MorrisJobke deleted the settings_users_ldap branch September 7, 2015 11:35
@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.

User management not accessible anymore
6 participants