Add recovery email parameter to MFA creation #82
Conversation
ethancanne
requested review from
briskt,
forevermatt,
mtompset and
jason-jackson
and removed request for
a team
features/request/RequestContext.php
Outdated
| if (empty($this->requestData['recovery_email'])) { | ||
| $this->getIdBrokerClient()->mfaCreate( | ||
| $this->requestData['employee_id'], | ||
| $this->requestData['type'], | ||
| $this->requestData['label'], | ||
| $this->rpOrigin, | ||
| ); | ||
| } else { | ||
| $this->getIdBrokerClient()->mfaCreate( | ||
| $this->requestData['employee_id'], | ||
| $this->requestData['type'], | ||
| $this->requestData['label'], | ||
| $this->rpOrigin, | ||
| $this->requestData['recovery_email'], | ||
| ); | ||
| } |
There was a problem hiding this comment.
Why not just do this?
| if (empty($this->requestData['recovery_email'])) { | |
| $this->getIdBrokerClient()->mfaCreate( | |
| $this->requestData['employee_id'], | |
| $this->requestData['type'], | |
| $this->requestData['label'], | |
| $this->rpOrigin, | |
| ); | |
| } else { | |
| $this->getIdBrokerClient()->mfaCreate( | |
| $this->requestData['employee_id'], | |
| $this->requestData['type'], | |
| $this->requestData['label'], | |
| $this->rpOrigin, | |
| $this->requestData['recovery_email'], | |
| ); | |
| } | |
| $this->getIdBrokerClient()->mfaCreate( | |
| $this->requestData['employee_id'], | |
| $this->requestData['type'], | |
| $this->requestData['label'], | |
| $this->rpOrigin, | |
| $this->requestData['recovery_email'], | |
| ); |
There was a problem hiding this comment.
The mfaCreate test without type: recovery was failing since it was adding a recovery_email: null into the body.
There was a problem hiding this comment.
I think it would be fine to add that to that test instead of doing if/else here.
src/IdBrokerClient.php
Outdated
| if ($recovery_email === null) { | ||
| $result = $this->mfaCreateInternal([ | ||
| 'employee_id' => $employee_id, | ||
| 'type' => $type, | ||
| 'label' => $label, | ||
| 'rpOrigin' => $rpOrigin, | ||
| ]); | ||
| } | ||
| else{ | ||
| $result = $this->mfaCreateInternal([ | ||
| 'employee_id' => $employee_id, | ||
| 'type' => $type, | ||
| 'label' => $label, | ||
| 'rpOrigin' => $rpOrigin, | ||
| 'recovery_email' => $recovery_email, | ||
| ]); | ||
| } | ||
|
|
There was a problem hiding this comment.
Any reason to not do this?
| if ($recovery_email === null) { | |
| $result = $this->mfaCreateInternal([ | |
| 'employee_id' => $employee_id, | |
| 'type' => $type, | |
| 'label' => $label, | |
| 'rpOrigin' => $rpOrigin, | |
| ]); | |
| } | |
| else{ | |
| $result = $this->mfaCreateInternal([ | |
| 'employee_id' => $employee_id, | |
| 'type' => $type, | |
| 'label' => $label, | |
| 'rpOrigin' => $rpOrigin, | |
| 'recovery_email' => $recovery_email, | |
| ]); | |
| } | |
| $result = $this->mfaCreateInternal([ | |
| 'employee_id' => $employee_id, | |
| 'type' => $type, | |
| 'label' => $label, | |
| 'rpOrigin' => $rpOrigin, | |
| 'recovery_email' => $recovery_email, | |
| ]); |
There was a problem hiding this comment.
I agree with you @jason-jackson but I don't see that as a reason to withhold PR approval.
There was a problem hiding this comment.
Or maybe it was for the null issue.
|
| ]); | ||
| } | ||
|
|
||
| var_dump($recovery_email); |
There was a problem hiding this comment.
Looks like a var_dump got left behind.
There was a problem hiding this comment.
Oh I'm glad you noticed that, I'll make sure to remove that
This PR adds support for including a recovery email when creating MFA methods via the
mfaCreatefunction.Addition
IdBrokerClient.phpto include a new recovery_email parameter.id-broker-api.phpto define the newrecovery_emailfield as an optional string in the request body.recoverytype to the enum list of valid MFA types.(request.feature)to include a test scenario for therecovery_emailfield.