Initial rewrite for changes in the Bootstrapping module #18
Conversation
There was a problem hiding this comment.
Hey @Firesphere, I have no experience with these modules at all yet, but I've had a quick skim over the changes and have left a few minor comments here and there. Nice work!
| - "**.php" | ||
| exclude_paths: | ||
| - docs/* | ||
| - tests/* |
There was a problem hiding this comment.
We don't use codeclimate as a part of our regular CI toolchain at the moment
There was a problem hiding this comment.
Fair enough, it's just a copy-paste from other modules. I don't mind removing it
| return TOTPLoginHandler::create($link, $this); | ||
| $algorithm = self::config()->get('algorithm'); | ||
|
|
||
| return in_array(strtolower($algorithm), ['sha1', 'sha256', 'sha512']) |
There was a problem hiding this comment.
You could make this either a config array OR a class constant (if we don't want it to be configurable)
There was a problem hiding this comment.
I'd go with constants, but I think it came from a change @elliot-sawyer made somewhere else
|
|
||
| return in_array(strtolower($algorithm), ['sha1', 'sha256', 'sha512']) | ||
| ? $algorithm | ||
| : 'sha1'; |
There was a problem hiding this comment.
If you did go with the above, then this could also be a constant e.g. self::ALGORITHM_DEFAULT
| } | ||
|
|
||
| /** | ||
| * @param $data | ||
| * @param HTTPRequest $request | ||
| * @param $token |
There was a problem hiding this comment.
Minor point: missing the type
| /** | ||
| * @param BootstrapMFALoginHandler $controller | ||
| * @param string $name | ||
| * @return TOTPForm|BootstrapMFALoginForm |
There was a problem hiding this comment.
This could possibly benefit from being a bit tighter. I see two possibilities:
TOTPFormis changed to extendBootstrapMFALoginForm- We use
LoginFormas the return hint instead
| $fields->push(PasswordField::create('token', _t(self::class . '.TOTPCODE', 'TOTP Code'))); | ||
| $fields = FieldList::create([ | ||
| PasswordField::create('token', _t(self::class . '.TOTPCODE', 'TOTP Code')), | ||
| HiddenField::create('AuthenticationMethod', $this->authenticator_class) |
There was a problem hiding this comment.
FYI I've made a PR to add a getAuthenticatorClass() method to use here instead of accessing this property directly: silverstripe/silverstripe-framework#8571
Won't be useful until this module requires 4.4 or newer though
There was a problem hiding this comment.
Looks good, all things are only minor from what we can see (@NightJar @ScopeyNZ pair peer review)
Seems like a first step as it is, as the BootstrapMFA is changing it's API to be more flexible.
At some point we will need to stop and take stock of both modules (and other MFA Authenticators) together, so we are happy for this to be merged now, and have follow-ups later.
All open unresolved comments should be converted to issues first before merging, except this one, which should just be done now: https://github.com/elliot-sawyer/totp-authenticator/pull/18/files?utf8=%E2%9C%93&diff=split#r230733713
| { | ||
| return TOTPLoginHandler::create($link, $this); | ||
| $algorithm = self::config()->get('algorithm'); |
There was a problem hiding this comment.
This should probably be static::config() to allow for late static binding & the ability for subclasses to have their own specific config.
|
There's also a few merge conflicts that need to be resolved |
Firesphere
force-pushed
the
rewrite-for-bootstrap-changes
branch
from
0db94f8 to
b960f46
Compare
This is awaiting the merge on the bootstrap module