Move default admin #7026
Move default admin #7026
Conversation
…thenticator, unifying and removing duplicate code.
tractorcow
force-pushed
the
move_default_admin
branch
from
19a175a
to
ff0b704
Compare
| ->first(); | ||
| } | ||
|
|
||
| // Validate against member if possible | ||
| if ($member && !$asDefaultAdmin) { | ||
| $result = $member->checkPassword($data['Password']); | ||
| $member->checkPassword($data['Password'], $result); |
There was a problem hiding this comment.
$this->checkPassword()
tractorcow
force-pushed
the
move_default_admin
branch
from
ff0b704
to
a549b70
Compare
| @@ -350,8 +308,17 @@ public function checkPassword($password) | |||
| */ | |||
| public function isDefaultAdmin() | |||
| { | |||
| return Security::has_default_admin() | |||
| && $this->Email === Security::default_admin_username(); | |||
| return DefaultAdminService::isDefaultAdmin($this->Email); | |||
There was a problem hiding this comment.
Should have a deprecation notice?
| * @param ValidationResult $result | ||
| * @return ValidationResult | ||
| */ | ||
| public function checkPassword(Member $member, $password, ValidationResult $result = null) |
There was a problem hiding this comment.
$result is immediately overwritten
There was a problem hiding this comment.
It's also passed into validateCanLogin() before being overwritten, so it's safe.
|
Aside from a little change I requested on Damian's update, this looks good for me to go and would wrap up everything on possible Authentication improvements. |
API Improve validation of authentication process
tractorcow
force-pushed
the
move_default_admin
branch
from
a549b70
to
62d0953
Compare
| "The provided details don't seem to be correct. Please try again." | ||
| )); | ||
| $member = DefaultAdminService::singleton()->findOrCreateDefaultAdmin(); | ||
| $member->validateCanLogin($result); |
There was a problem hiding this comment.
$result = $member->validateCanLogin($result);?
I'm not sure if this call does anything otherwise
There was a problem hiding this comment.
I believe it should indeed be that.
There was a problem hiding this comment.
Damian has refactored validateCanLogin(&$result) which was pretty much what I had asked :D a consistent method signature
There was a problem hiding this comment.
Ah, right. Agreed, I'm good :)
|
After Damian makes the signature update, I'm happy to merge (or he can if it's green before I get home) |
|
Ok, pushed up fix with your feedback @flamerohr |
| @@ -0,0 +1,4 @@ | |||
| Stack trace: | |||
There was a problem hiding this comment.
Was this file really needed ;)
There was a problem hiding this comment.
hey...! did not notice it :(
There was a problem hiding this comment.
Hmm - it looks like it's been removed at some point from the main repo - so all good
There was a problem hiding this comment.
I removed it manually, sorry for the mistake!
There was a problem hiding this comment.
That was my mistake, swiftly pushing because Damian wanted to take over. Sorry!
There was a problem hiding this comment.
no problem - it's gone now so allllll good. :)
Required for #6387