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.

Sign up for GitHub

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

Jump to bottom

API Add change password functionality via LDAP server #5

Merged
merged 2 commits into from
Oct 16, 2017
Merged

API Add change password functionality via LDAP server #5

merged 2 commits into from
Oct 16, 2017

Conversation

robbieaverill
Copy link
Contributor

@robbieaverill robbieaverill commented Oct 4, 2017 •
edited
Loading

Requirements

Changes

robbieaverill
robbieaverill commented Oct 4, 2017
View reviewed changes
docs/en/developer.md
$member->FirstName = 'Joe';
$member->Username = 'jbloggs';
$member->write();
```
Copy link
Contributor Author

@robbieaverill robbieaverill Oct 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

More extensive docs rewrite is coming next edit #6

@robbieaverill
Copy link
Contributor Author

robbieaverill commented Oct 4, 2017 •
edited
Loading

SS core doesn't trigger Member::changePassword when you update a password in a member profile via the admin interface. It would be ideal if it did, but it's only being used in the "forgot my password" type login forms at the moment. @halkyon @mateusz what are your thoughts on this? Log a separate issue/PR to core to make it use changePassword/add a hack to this module instead?

Edit: I've logged this on framework for now - silverstripe/silverstripe-framework#7435

@robbieaverill robbieaverill changed the title API Add change password functionality via LDAP server WIP: API Add change password functionality via LDAP server Oct 4, 2017
@robbieaverill robbieaverill mentioned this pull request Oct 5, 2017
Merged
@robbieaverill robbieaverill changed the title WIP: API Add change password functionality via LDAP server API Add change password functionality via LDAP server Oct 10, 2017
@NightJar NightJar merged commit 432a3b1 into silverstripe:master Oct 16, 2017
@NightJar NightJar deleted the pulls/1.0/fix-change-password branch October 16, 2017 20:38
ichaber
ichaber reviewed Oct 17, 2017
View reviewed changes
src/Authenticators/LDAPAuthenticator.php
$result = $result ?: ValidationResult::create();

/** @var LDAPService $service */
$service = Injector::inst()->get(LDAPService::class);
Copy link

Choose a reason for hiding this comment

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

We could replace that via Setter Dependency Injection to get the server and allow a custom LDAP Service.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, #7

tests/php/Authenticators/LDAPChangePasswordHandlerTest.php
use SilverStripe\Control\Session;
use SilverStripe\Core\Injector\Injector;
use SilverStripe\Dev\SapphireTest;
use SIlverStripe\LDAP\Authenticators\LDAPAuthenticator;
Copy link

Choose a reason for hiding this comment

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

That should be use SilverStripe\LDAP\Authenticators\LDAPAuthenticator; (with a lowercase i)

ichaber
ichaber reviewed Oct 17, 2017
View reviewed changes
tests/php/Authenticators/LDAPChangePasswordHandlerTest.php
use SilverStripe\Core\Injector\Injector;
use SilverStripe\Dev\SapphireTest;
use SIlverStripe\LDAP\Authenticators\LDAPAuthenticator;
use SIlverStripe\LDAP\Authenticators\LDAPChangePasswordHandler;
Copy link

Choose a reason for hiding this comment

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

That should be use SilverStripe\LDAP\Authenticators\LDAPChangePasswordHandler; (with a lowercase i)

ichaber
ichaber reviewed Oct 17, 2017
View reviewed changes
tests/php/Authenticators/LDAPChangePasswordHandlerTest.php
use SilverStripe\Dev\SapphireTest;
use SIlverStripe\LDAP\Authenticators\LDAPAuthenticator;
use SIlverStripe\LDAP\Authenticators\LDAPChangePasswordHandler;
use SIlverStripe\LDAP\Forms\LDAPChangePasswordForm;
Copy link

Choose a reason for hiding this comment

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

That should be use SilverStripe\LDAP\Forms\LDAPChangePasswordForm; (with a lowercase i)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#8

@robbieaverill robbieaverill mentioned this pull request Oct 17, 2017
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ichaber ichaber ichaber left review comments

@NightJar NightJar NightJar approved these changes

Assignees
No one assigned
Labels
affects/v4
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@robbieaverill @NightJar @ichaber