Skip to content

Conversation

@GitHubUser4234
Copy link
Contributor

@GitHubUser4234 GitHubUser4234 commented Jul 22, 2016

Please refer to owncloud/core#23992

@MorrisJobke
Copy link
Member

best would be to wait until this is integrated into owncloud, because we regularly fetch the changes from there. Otherwise we will fix stuff here differently from what is fixed in ownCloud and merging this then in the end will be a nightmare.

@GitHubUser4234 Is this okay for you? I would then close this PR.

@GitHubUser4234
Copy link
Contributor Author

@MorrisJobke: We switched to NC and it is high priority for us to have it included in NC 10. Waiting would put this target at risk. For OC we won't push for a merge there in the near future.

@MorrisJobke
Copy link
Member

@MorrisJobke: We switched to NC and it is high priority for us to have it included in NC 10.

We already released a beta release of NC 10 and will not include new features anymore in the stable10 branch. This needs to wait for NC 11

@MorrisJobke
Copy link
Member

cc @blizzz

@blizzz
Copy link
Member

blizzz commented Jul 22, 2016

@MorrisJobke: We switched to NC and it is high priority for us to have it included in NC 10.

We already released a beta release of NC 10 and will not include new features anymore in the stable10 branch. This needs to wait for NC 11

I am not aware we announced a feature freeze, did we? Also, because we still lack other planned features for 10.

@andreas-p
Copy link

AFAICS there's no setPassword functionality implemented yet?
I've written a somewhat hackyish app allowing the user to change his own password (userPassword with several crypt options including SSHA, as well as sambaNTPassword in case it's a sambaSamAccount) which I could contribute.

@GitHubUser4234
Copy link
Contributor Author

GitHubUser4234 commented Jul 23, 2016

@andreas-p : good news, a setPassword functionality is in the works: #520
Password hashing won't be in though because

  1. it prevents LDAP server password policy enforcement
  2. it can actually be done by the LDAP server itself

@andreas-p
Copy link

Well, if sambaNTpassword isn't set correctly in parallel it's unusable for me....
AFAIK there's no ldap_passwd or similar function, that's why I implemented hashing myself, necessary for Samba anyway.

@blizzz
Copy link
Member

blizzz commented Jul 26, 2016

Integration Tests succeeding ✅

$mocks['ocConfig'] = $this->getMock('\OCP\IConfig');
$mocks['db'] = $this->getMock('\OCP\IDBConnection');
$mocks['helper'] = $this->getMock('\OCA\User_LDAP\Helper');
$mocks['helper'] = new \OCA\User_LDAP\Helper();
Copy link
Member

Choose a reason for hiding this comment

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

this change makes the unit test break

@GitHubUser4234
Copy link
Contributor Author

thanks a lot @blizzz . Changes are committed accordingly. I have also removed the word "owncloud" from the comments of all new API methods.

@blizzz
Copy link
Member

blizzz commented Jul 27, 2016

LGTM

@blizzz
Copy link
Member

blizzz commented Jul 27, 2016

@MorrisJobke: We switched to NC and it is high priority for us to have it included in NC 10.

We already released a beta release of NC 10 and will not include new features anymore in the stable10 branch. This needs to wait for NC 11

I am not aware we announced a feature freeze, did we? Also, because we still lack other planned features for 10.

Discussed with @karlitschek earlier this week that a backport would be OK. Since we had no feature freeze, upon to the feature maintainers discretion.

Also, it has minor changes in the existing code, primarily extends Nc and the LDAP backend with an LDAP Provider functionality.

@blizzz
Copy link
Member

blizzz commented Jul 27, 2016

We need a second reviewer :) @MorrisJobke or @schiessle maybe?

@GitHubUser4234
Copy link
Contributor Author

As requested by @schiessle at #600 , new files are updated with nextcloud license headers now.

@schiessle
Copy link
Member

schiessle commented Jul 28, 2016

Can't test it but I'm sure @blizzz and @GitHubUser4234 know what they are doing 😉

Had a look at the code, looks good to me 👍

@GitHubUser4234 thanks for adjusting the copyright headers!

@blizzz blizzz merged commit 7331109 into nextcloud:master Jul 28, 2016
@GitHubUser4234
Copy link
Contributor Author

Thanks a lot @blizzz @schiessle @MorrisJobke for making this contribution possible 👍

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.

5 participants