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

Hide the LDAP password in the client side #25702

Merged
merged 1 commit into from Aug 11, 2016

Conversation

@jvillafanez
Copy link
Member

jvillafanez commented Aug 5, 2016

Connection checks will be done by using the configuration id, with the
stored password. LDAP password won't be sent to the client.

Fix owncloud/enterprise#1489

@DeepDiver1975 backports are required

@jvillafanez jvillafanez added this to the 9.2 milestone Aug 5, 2016
@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Aug 5, 2016

CLA assistant check
All committers have signed the CLA.

@mention-bot

This comment has been minimized.

Copy link

mention-bot commented Aug 5, 2016

@jvillafanez, thanks for your PR! By analyzing the annotation information on this pull request, we identified @blizzz, @nickvergessen and @Kondou-ger to be potential reviewers

@@ -318,7 +318,7 @@ OCA = OCA || {};
*/
requestConfigurationTest: function() {
var url = OC.generateUrl('apps/user_ldap/ajax/testConfiguration.php');
var params = OC.buildQueryString(this.configuration);
var params = OC.buildQueryString({ldap_serverconfig_chooser: this.configID});

This comment has been minimized.

Copy link
@PVince81

PVince81 Aug 8, 2016

Member

Does it mean that ldap_serverconfig_chooser is all we need from this.configuration ? This looks slightly suspicious

This comment has been minimized.

Copy link
@jvillafanez

jvillafanez Aug 9, 2016

Author Member

Yes, we'll test it against the stored configuration, not against whatever is showing.

@PVince81

This comment has been minimized.

Copy link
Member

PVince81 commented Aug 8, 2016

Tested, works 👍
After refreshing the page, the password is replaced properly and the connection check and other checks still work fine.

@jvillafanez please solve the conflicts

Connection checks will be done by using the configuration id, with the
stored password. LDAP password won't be sent to the client.
@jvillafanez jvillafanez force-pushed the ldap_hide_configured_password branch from ed09e47 to 7641292 Aug 9, 2016
@jvillafanez

This comment has been minimized.

Copy link
Member Author

jvillafanez commented Aug 9, 2016

Rebased

@@ -31,4 +31,9 @@
$prefix = (string)$_POST['ldap_serverconfig_chooser'];
$ldapWrapper = new OCA\User_LDAP\LDAP();
$connection = new \OCA\User_LDAP\Connection($ldapWrapper, $prefix);
OCP\JSON::success(array('configuration' => $connection->getConfiguration()));
$configuration = $connection->getConfiguration();
if (isset($configuration['ldap_agent_password']) && $configuration['ldap_agent_password'] !== '') {

This comment has been minimized.

Copy link
@butonic

butonic Aug 11, 2016

Member

use !empty()? well that will also return true if $configuration['ldap_agent_password'] is "0". hmm nevermind

@PVince81

This comment has been minimized.

Copy link
Member

PVince81 commented Aug 11, 2016

Pffff... another case where Jenkins couldn't check out the code.

As this code isn't covered by any tests, merging directly.

@PVince81 PVince81 merged commit 2b29177 into master Aug 11, 2016
3 of 4 checks passed
3 of 4 checks passed
Jenkins This commit cannot be built
Details
Scrutinizer 160 new issues, 65 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@PVince81 PVince81 deleted the ldap_hide_configured_password branch Aug 11, 2016
@PVince81

This comment has been minimized.

Copy link
Member

PVince81 commented Aug 11, 2016

@jvillafanez please backport

@jvillafanez

This comment has been minimized.

Copy link
Member Author

jvillafanez commented Aug 22, 2016

@PVince81 do we need more backports? Original issue was for 9.0 so I guess we don't need more.

@PVince81

This comment has been minimized.

Copy link
Member

PVince81 commented Aug 22, 2016

I think it's fine to stop here as it's not critical

@lock

This comment has been minimized.

Copy link

lock bot commented Aug 4, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants
You can’t perform that action at this time.